Context & catalog isolation — ExecContext gains Backend, handlers decoupled from mpr#229
Conversation
…ntext, populate Backend on connect
… ctx.Field in free functions
Demonstrate that handlers can now be tested without a real .mpr file by using MockBackend with a pre-populated ContainerHierarchy cache. Two tests verify table output and module filtering.
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationApprove. This PR successfully refactors the executor layer to improve decoupling, testability, and maintainability without altering functionality, following the project's architectural guidelines. The changes are well-scoped, consistently applied, and verified. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
This PR continues the executor refactoring by making ExecContext the primary carrier of domain dependencies (via a Backend interface and related fields) and migrating handlers away from direct .mpr reader/writer usage, while also isolating catalog building behind an interface to reduce coupling and improve testability.
Changes:
- Extend
ExecContextwithBackend,MprPath,SqlMgr,ThemeRegistry,Settings, and connection helper methods (Connected()/ConnectedForWrite()), then propagate these fromExecutor. - Migrate many handlers/utilities from
e.reader/e.writerande.*fields toctx.Backend/ctx.*equivalents; convert some name lookups to free functions. - Introduce
catalogisolation via a reader interface and add a mock-backend based handler test (showEnumerations).
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mdl/executor/validate.go | Switch validation connection checks to ctx.Connected() and remove executor backrefs. |
| mdl/executor/oql_type_inference.go | Use ctx.Backend for entity/domain model lookup. |
| mdl/executor/hierarchy.go | Gate hierarchy building on ctx.Connected() and use context cache. |
| mdl/executor/helpers.go | Replace reader/writer calls with ctx.Backend and use ConnectedForWrite(). |
| mdl/executor/format.go | Keep executor output in sync during capture paths that still write via executor internals. |
| mdl/executor/executor_query.go | Use ctx.Connected() for SHOW/DESCRIBE connection gating. |
| mdl/executor/executor_dispatch.go | Populate new ExecContext fields from the executor (Backend, MprPath, etc.). |
| mdl/executor/executor_connect.go | Populate e.backend via mprbackend.Wrap and use ConnectedForWrite() checks. |
| mdl/executor/executor.go | Add backend.FullBackend to Executor and convert name lookup helpers to free functions using ctx. |
| mdl/executor/exec_context.go | Add Backend/MprPath/SqlMgr/ThemeRegistry/Settings + helper methods. |
| mdl/executor/cmd_workflows_write.go | Migrate workflow write operations to ctx.Backend. |
| mdl/executor/cmd_workflows.go | Migrate workflow read paths to ctx.Backend. |
| mdl/executor/cmd_widgets.go | Migrate widget raw unit read/write to ctx.Backend and connection checks to ctx. |
| mdl/executor/cmd_styling.go | Use ctx.MprPath and ctx.Backend for styling operations. |
| mdl/executor/cmd_structure.go | Replace reader-based counting with ctx.Backend calls. |
| mdl/executor/cmd_snippets.go | Use ctx.Backend.ListSnippets(). |
| mdl/executor/cmd_settings.go | Use ctx.Backend for project settings read/write. |
| mdl/executor/cmd_security.go | Use ctx.Backend for security reads and ctx.Format for format branching. |
| mdl/executor/cmd_rest_clients.go | Use ctx.Backend for REST client read/write operations. |
| mdl/executor/cmd_rename.go | Route rename/reference update operations through ctx.Backend. |
| mdl/executor/cmd_published_rest.go | Use ctx.Backend for published REST operations and ctx.Quiet for messaging. |
| mdl/executor/cmd_pages_show.go | Use ctx.Backend.ListPages(). |
| mdl/executor/cmd_pages_describe_parse.go | Use ctx.Backend for snippet/module resolution in parsing. |
| mdl/executor/cmd_pages_describe_output.go | Use ctx.Backend for page ID → qualified name resolution. |
| mdl/executor/cmd_pages_describe.go | Use ctx.Backend for pages/snippets/layouts/raw-unit reads. |
| mdl/executor/cmd_pages_create_v3.go | Use ctx.Backend for page/snippet create/update/delete and ctx.GetThemeRegistry(). |
| mdl/executor/cmd_pages_builder.go | Use ctx.Backend for drop page/snippet operations. |
| mdl/executor/cmd_page_wireframe.go | Use ctx.Backend for wireframe generation data access. |
| mdl/executor/cmd_odata.go | Use ctx.Backend for OData and external entity operations. |
| mdl/executor/cmd_navigation.go | Use ctx.Backend for navigation read/update. |
| mdl/executor/cmd_move.go | Route move operations and reference updates through ctx.Backend. |
| mdl/executor/cmd_modules.go | Migrate module operations and bulk deletes to ctx.Backend. |
| mdl/executor/cmd_module_overview.go | Gate on ctx.Connected() for module overview generation. |
| mdl/executor/cmd_misc.go | Use ctx.MprPath for UPDATE/REFRESH and ctx.Settings for SET. |
| mdl/executor/cmd_microflows_show.go | Use ctx.Backend and new name lookup free functions for DESCRIBE MICROFLOW. |
| mdl/executor/cmd_microflows_format_action.go | Use ctx.Connected()/ctx.Backend for legacy page ID lookup paths. |
| mdl/executor/cmd_microflows_drop.go | Use ctx.Backend for microflow deletion and ctx.Cache for session tracking. |
| mdl/executor/cmd_microflows_create.go | Use ctx.Backend for read/write paths during microflow creation. |
| mdl/executor/cmd_microflow_elk.go | Use ctx.Backend for microflow ELK data access. |
| mdl/executor/cmd_mermaid.go | Use ctx.Backend for mermaid generation data access. |
| mdl/executor/cmd_lint.go | Use ctx.Connected() and ctx.MprPath for lint configuration and connectivity. |
| mdl/executor/cmd_layouts.go | Use ctx.Backend.ListLayouts(). |
| mdl/executor/cmd_jsonstructures.go | Use ctx.Backend for JSON structure operations. |
| mdl/executor/cmd_javascript_actions.go | Use ctx.Backend for JS action reads and ctx.MprPath for source lookup. |
| mdl/executor/cmd_javaactions.go | Use ctx.Backend for Java action reads/writes and ctx.MprPath for source lookup. |
| mdl/executor/cmd_import_mappings.go | Use ctx.Backend for import mapping operations. |
| mdl/executor/cmd_import.go | Use ctx.Backend for settings/domain model reads in import flows. |
| mdl/executor/cmd_imagecollections.go | Use ctx.Backend for image collection operations. |
| mdl/executor/cmd_fragments.go | Use ctx.Backend to load page/snippet containers for fragment extraction. |
| mdl/executor/cmd_folders.go | Use ctx.Backend for folder list/move/delete. |
| mdl/executor/cmd_features.go | Use ctx.Connected() and ctx.Backend.ProjectVersion(). |
| mdl/executor/cmd_export_mappings.go | Use ctx.Backend for export mapping operations. |
| mdl/executor/cmd_enumerations_mock_test.go | Add mock-backend based tests for showEnumerations. |
| mdl/executor/cmd_enumerations.go | Use ctx.Backend for enumeration list/create/delete/describe. |
| mdl/executor/cmd_entities_describe.go | Use ctx.Backend for entity listing and microflow lookup helpers. |
| mdl/executor/cmd_entities_access.go | Use ctx.Backend when re-reading domain model after changes. |
| mdl/executor/cmd_entities.go | Use ctx.Backend for entity/view-entity create/alter/drop flows and catalog querying via ctx.Catalog. |
| mdl/executor/cmd_domainmodel_elk.go | Use ctx.Backend for ELK generation data access. |
| mdl/executor/cmd_diff_local.go | Use ctx.Backend for version/contents directory and name lookups. |
| mdl/executor/cmd_diff.go | Use ctx.Connected() gating and ctx.Backend for domain model/microflow comparisons. |
| mdl/executor/cmd_dbconnection.go | Use ctx.Backend for database connection operations. |
| mdl/executor/cmd_datatransformer.go | Use ctx.Backend for data transformer list/create/drop and ctx.Quiet messaging. |
| mdl/executor/cmd_contract.go | Use ctx.Backend for contract-related model access and writes. |
| mdl/executor/cmd_constants.go | Use ctx.Backend for constants and project settings access. |
| mdl/executor/cmd_catalog.go | Use ctx.MprPath and ctx.Backend for catalog caching and pre-warm operations. |
| mdl/executor/cmd_businessevents.go | Use ctx.Backend for business event service operations. |
| mdl/executor/cmd_associations.go | Use ctx.Backend for association create/alter/drop/show/describe paths. |
| mdl/executor/cmd_alter_workflow.go | Use ctx.Backend for raw workflow BSON read/write. |
| mdl/executor/cmd_alter_page.go | Use ctx.Backend for raw page/snippet BSON read/write and ctx.GetThemeRegistry(). |
| mdl/executor/cmd_agenteditor_models.go | Use ctx.Backend for agent editor model listing/lookup. |
| mdl/executor/cmd_agenteditor_mcpservices.go | Use ctx.Backend for agent editor MCP service listing/lookup. |
| mdl/executor/cmd_agenteditor_kbs.go | Use ctx.Backend for agent editor KB listing/lookup. |
| mdl/executor/cmd_agenteditor_agents.go | Use ctx.Backend for agent editor agent listing/lookup. |
| mdl/executor/autocomplete.go | Use ctx.Connected() and ctx.Backend across autocomplete sources. |
| mdl/backend/mpr/backend.go | Add Wrap(*mpr.Writer, path) helper and expose ListJavaActionsFull(). |
| mdl/backend/mock/mock_java.go | Add mock implementation hook for ListJavaActionsFull(). |
| mdl/backend/mock/backend.go | Add ListJavaActionsFullFunc field to MockBackend. |
| mdl/backend/java.go | Extend Java backend interface with ListJavaActionsFull(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewWhat Looks Good
RecommendationApprove. This is a well-executed refactor that successfully decouples the executor from direct Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewWhat Looks GoodThis PR represents a well-executed refactoring effort that successfully decouples handlers from direct executor dependencies while improving testability. The changes are mechanical and consistent across the codebase:
The refactoring maintains backward compatibility while setting up better abstractions for future changes. Error handling patterns are preserved through the RecommendationApprove this PR. It successfully accomplishes its stated goals of executor refactoring and catalog isolation without introducing functional changes, improves testability through interface-based design, and follows the project's architectural direction. The changes are mechanical, consistent, and well-tested. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/context-isolation
branch
from
9730b80 to
790f63b
Compare
AI Code ReviewLet me review this PR against the checklist. This PR is part of an executor refactoring series that aims to decouple handlers from direct executor fields and use the ExecContext instead. First, I'll look for any critical issues: Critical IssuesLooking through the diff, I don't see any obvious critical bugs or correctness issues. The changes are primarily replacing direct executor field accesses (e.g., Let me check for any specific issues:
This is consistent with the PR description. Let me check for any potential issues: In - builder := catalog.NewBuilder(cat, e.reader)
+ builder := catalog.NewBuilder(cat, ctx.Backend)And in -func NewBuilder(catalog *Catalog, reader *mpr.Reader) *Builder {
+func NewBuilder(catalog *Catalog, reader CatalogReader) *Builder {This is good - they're introducing a Let me check if there are any places where the refactoring might be incomplete: Looking through the diff, I see many consistent replacements. The PR mentions that ~27 Let me check for any MDL syntax changes - I don't see any changes to MDL grammar, AST, or executor handlers that would introduce new MDL syntax. This appears to be purely a refactoring of the executor implementation. Now let me check the checklist items: Overlap & duplication
MDL syntax design
Full-stack consistency
Test coverage
Security & robustness
Scope & atomicity
Code quality
Let me double-check a few specific areas: In In In - e := ctx.executor
-
- constants, err := e.reader.ListConstants()
+ constants, err := ctx.Backend.ListConstants()is consistently applied and correct. One thing I should check: in the -func preWarmCache(ctx *ExecContext) {
- e := ctx.executor
- h, _ := getHierarchy(ctx)
- if h == nil || ctx.Cache == nil {
- return
- }
-
- // Build entity name lookup
- ctx.Cache.entityNames = make(map[model.ID]string)
- dms, _ := e.reader.ListDomainModels()
+func preWarmCache(ctx *ExecContext) {
+ h, _ := getHierarchy(ctx)
+ if h == nil || ctx.Cache == nil {
+ return
+ }
+
+ // Build entity name lookup
+ ctx.Cache.entityNames = make(map[model.ID]string)
+ dms, _ := ctx.Backend.ListDomainModels()This looks correct - they removed the unnecessary Let me check if there are any remaining instances of the old pattern that should have been converted: Scanning through the diff, I don't see any obvious missed conversions. The PR author mentioned that ~27 lines remain for legitimate reasons, which seems plausible for cases where they need direct access to the executor for specific methods like Let me check if the changes maintain the same behavior: The key change is replacing From the PR description: "ExecContext gains domain-specific fields: Backend, MprPath, SqlMgr, ThemeRegistry, Settings, Connected()/ConnectedForWrite() methods" And: "Replace 154 reader/writer nil checks with Connected()/ConnectedForWrite()" This suggests that Similarly, This is a good improvement as it encapsulates the connection state checking in the ExecContext rather than having each handler check for nil reader/writer. Let me look for any potential issues with the MockBackend: In ListJavaActionsFullFunc func() ([]*javaactions.JavaAction, error)And in func (m *MockBackend) ListJavaActionsFull() ([]*javaactions.JavaAction, error) {
if m.ListJavaActionsFullFunc != nil {
return m.ListJavaActionsFullFunc()
}
return nil, nil
}This follows the same pattern as the other mock methods and looks correct. In func (b *MprBackend) ListJavaActionsFull() ([]*javaactions.JavaAction, error) {
return b.reader.ListJavaActionsFull()
}Assuming the Let me check if there are any missing interface updates: The JavaBackend interface in ListJavaActionsFull() ([]*javaactions.JavaAction, error)This is implemented in both the mock backend and the MPR backend, so it should be fine. Now, let me check if there are any places where they might have incorrectly replaced something: In - oldOutput := e.output
- e.output = &buf
+ oldOutput := ctx.Output
+ ctx.Output = &bufThis looks correct - they're saving and restoring the output buffer using the context. In - if e.reader.Version() != 2 {
+ if ctx.Backend.Version() != 2 {Assuming the Backend interface has a Version() method, this is correct. Let me check if the Backend interface actually has these methods: From the PR description, ExecContext gained a Backend field, and they're replacing e.reader/e.writer calls with ctx.Backend. Looking at the changes, it seems the Backend interface now includes methods like:
And also methods like:
This makes sense as the Backend is meant to encapsulate the data access layer. One thing I should verify: in several places, they're using Let me check if there are any inconsistencies: In - if e.reader == nil {
+ if !ctx.Connected() {
return mdlerrors.NewNotConnected()
}
- if e.writer == nil {
+ if !ctx.ConnectedForWrite() {
return mdlerrors.NewNotConnectedWrite()
}This correctly separates the read and write connection checks. In - rawBytes, err := e.reader.GetRawUnitBytes(unitID)
+ rawBytes, err := ctx.Backend.GetRawUnitBytes(unitID)And: - if err := e.writer.UpdateRawUnit(string(unitID), outBytes); err != nil {
+ if err := ctx.Backend.UpdateRawUnit(string(unitID), outBytes); err != nil {This suggests that the Backend interface has both GetRawUnitBytes (reader) and UpdateRawUnit (writer) methods, which makes sense. Let me check if there are any places where they might have missed converting something: Scanning through the diff, I see many consistent patterns. For example, in - e := ctx.executor
- if e.reader == nil {
+ if !ctx.Connected() {
return mdlerrors.NewNotConnected()
}
- agents, err := e.reader.ListAgentEditorAgents()
+ agents, err := ctx.Backend.ListAgentEditorAgents()This is correct. I think the refactoring is well-executed and consistent. Let me check if there are any issues with the test changes: In - if err := applyDropBoundaryEvent(doc, op); err != nil {
+ if err := applyDropBoundaryEvent(io.Discard, doc, op); err != nil {This is correct given the function signature change. Now, let me consider if there are any broader architectural concerns: The PR is part of a series refactoring the executor to improve testability and decouple concerns. This specific PR:
This aligns well with the project's architecture goals mentioned in CLAUDE.md. Let me check if there are any violations of the MDL syntax design guidelines - since no MDL syntax is being added or modified, there's nothing to check here. For full-stack consistency: not applicable since no new MDL features. Test coverage: The PR mentions mock-based tests for showEnumerations handler, and I see test updates in the diff, so this seems addressed. Security & robustness: The changes improve encapsulation by reducing direct access to executor internals, which is a positive for robustness. Scope & atomicity: The PR is focused on executor refactoring, which is a single concern. Code quality: The refactoring appears consistent and complete. Let me do one final check for any obvious mistakes: In -func getCachePath(ctx *ExecContext) string {
- e := ctx.executor
- if e.mprPath == "" {
+ if ctx.MprPath == "" {
return ""
}
- dir := filepath.Dir(e.mprPath)
+ dir := filepath.Dir(ctx.MprPath)This looks correct - they removed the unnecessary executor variable and used ctx.MprPath directly. In -func getMprModTime(ctx *ExecContext) time.Time {
- e := ctx.executor
- if e.mprPath == "" {
+ if ctx.MprPath == "" {
return time.Time{}
}
- info, err := os.Stat(e.mprPath)
+ info, err := os.Stat(ctx.MprPath)Also correct. In -func isCacheValid(ctx *ExecContext, cachePath string, requiredMode string) (bool, string) {
- e := ctx.executor
// Check if cache file exists
if _, err := os.Stat(cachePath); os.IsNotExist(err) {
return false, "cache file does not exist"
}
// Check MPR path matches
- if info.MprPath != e.mprPath {
+ if info.MprPath != ctx.MprPath {Correct. In - version, _ := e.reader.GetMendixVersion()
+ version, _ := ctx.Backend.GetMendixVersion()And: - builder := catalog.NewBuilder(cat, e.reader)
+ builder := catalog.NewBuilder(cat, ctx.Backend)Correct. -cat.SetCacheInfo(e.mprPath, getMprModTime(ctx), version, buildMode, elapsed)
+cat.SetCacheInfo(ctx.MprPath, getMprModTime(ctx), version, buildMode, elapsed)Correct. In -func execRefreshCatalogStmt(ctx *ExecContext, stmt *ast.RefreshCatalogStmt) error {
- e := ctx.executor
- if e.reader == nil {
+ if !ctx.Connected() {
return mdlerrors.NewNotConnected()
}Correct. In - // Create a goroutine-local executor: shared reader + cache, own output buffer.
- // TODO: Replace with ExecContext.Fork() when MR 3 makes ExecContext self-contained.
+ // Create a goroutine-local context: shared backend + cache, own output buffer.
var buf bytes.Buffer
local := &Executor{
reader: ctx.executor.reader,
@@ -700,8 +692,14 @@ func captureDescribeParallel(ctx *ExecContext, objectType string, qualifiedName
cache: ctx.Cache,
}
localCtx := &ExecContext{
+ Context: ctx.Context,
Output: &buf,
+ Format: ctx.Format,
+ Quiet: ctx.Quiet,
+ Logger: ctx.Logger,
+ Backend: ctx.Backend,
Cache: ctx.Cache,
+ MprPath: ctx.MprPath,
executor: local,
}This is interesting - they're creating a local Executor but setting its reader to The local ExecContext is properly initialized with:
This looks correct for their intermediate state. In -func preWarmCache(ctx *ExecContext) {
- e := ctx.executor
h, _ := getHierarchy(ctx)
if h == nil || ctx.Cache == nil {
return
}
// Build entity name lookup
ctx.Cache.entityNames = make(map[model.ID]string)
- dms,
---
*Automated review via OpenRouter (Nemotron Super 120B) — [workflow source](https://github.com/mendixlabs/mxcli/blob/main/.github/workflows/ai-review.yml)* |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/context-isolation
branch
from
790f63b to
b46f0bf
Compare
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. This PR successfully executes the executor refactoring goals outlined in the summary, improving testability and decoupling through proper interface usage while maintaining full backward compatibility. The changes are mechanical but significant, applied consistently across the codebase, and verified to work correctly. No syntax or behavioral changes to MDL features were made, so the full-stack consistency checklist doesn't apply. The refactor achieves its intended architectural improvements without introducing regressions. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/context-isolation
branch
from
b46f0bf to
974d94c
Compare
AI Code ReviewOkay, let me review this pull request. The user has provided a detailed PR summary and the diff, and I need to review it against the checklist in CLAUDE.md. First, I'll look at the PR summary to understand what changes are being made: This is the third PR in the executor refactoring series. Key changes:
Now, I need to check the diff against the review checklist. The checklist has several sections: Overlap & duplication, MDL syntax design, Full-stack consistency, Test coverage, Security & robustness, Scope & atomicity, Code quality, Bugs & correctness. Since this is a refactoring PR (not adding new MDL syntax), I should focus on whether the refactoring is done correctly according to the architecture guidelines. Let me scan through the diff to see what's changed:
Let me check against the checklist: Overlap & duplication
MDL syntax design
Full-stack consistency
Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Now, let me check for any potential issues:
Looking at the backend definitions:
The PR summary says: "ExecContext gains domain-specific fields: Backend, MprPath, SqlMgr, ThemeRegistry, Settings" And in the diff, I see in various files:
So it seems the Backend interface is meant to encapsulate all the backend operations. Let me check if there are any missing method implementations that would cause compilation errors. Since the PR says "make build — clean", I'll assume it compiles. One thing I noticed: in mdl/executor/cmd_catalog.go, there's a comment: And they changed it to: This shows they're in the process of making ExecContext self-contained, which aligns with the PR goal. Now, for the checklist items that don't apply (MDL syntax design, Full-stack consistency for MDL features), I should mark them as not applicable since this PR doesn't touch MDL syntax. Let me structure my review: Critical Issues: None found Let me look for any minor issues:
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
mdl/executor/hierarchy.go:128
getHierarchynow gates onctx.Connected(), but it still dereferencesctx.executor/e.readerwhen the hierarchy cache isn’t already populated. This can panic for contexts constructed without anExecutor(e.g., tests usingMockBackend) or for future non-MPR backends whereBackendis connected bute.readeris nil.
Consider either (a) building the hierarchy from ctx.Backend (ListModules/ListUnits/ListFolders) instead of e.reader, or (b) adding a defensive guard like if ctx.executor == nil || ctx.executor.reader == nil { return nil, mdlerrors.NewNotConnected() } before calling NewContainerHierarchy.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/context-isolation
branch
from
974d94c to
9f02d5d
Compare
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the pull request. This is a well-executed refactor that improves code quality, testability, and separation of concerns while maintaining full backward compatibility and passing all tests. The changes are consistently applied, properly scoped, and represent a significant step toward making the executor logic more modular and easier to test. No changes are needed. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
mdl/executor/executor.go:318
- Executor.Close() now clears e.backend but does not close or clear other long-lived resources like the catalog. If a catalog was built/loaded, this leaves the DB handle open and may keep stale state around for any reuse. Suggestion: in Close(), if e.catalog != nil call e.catalog.Close() and set it to nil (and consider resetting ThemeRegistry as well).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/context-isolation
branch
from
9f02d5d to
29698c1
Compare
AI Code ReviewWhat Looks Good
RecommendationApprove. This is a well-executed refactor that improves the codebase's structure and testability without changing behavior. The PR is properly scoped to a single refactoring goal, consistently implemented, and all verification checks pass. The refactor continues the series logically (#222 → #224 → #225 → this PR) and maintains backward compatibility while enabling better unit testing through interface abstraction. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
mdl/executor/hierarchy.go:123
- When initializing the cache, this writes through to e.cache without checking whether ctx.executor is non-nil. Since ExecContext can be constructed without an Executor (see mock-based tests), add a nil-check before assigning to e.cache (or stop syncing cache to Executor in this helper).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tput threading, disconnect ctx cleanup, cache invalidation, theme registry persistence, parallel ctx fields, settings persistence, structured logging, nil-safety guards, project-scoped cache reset, backend-based hierarchy, enum ref error handling
retran
force-pushed
the
feature/context-isolation
branch
from
29698c1 to
7fe3fb5
Compare
AI Code ReviewWhat Looks GoodThis PR continues the executor refactoring series successfully:
The refactoring follows the established pattern from previous PRs (#222, #224, #225) and maintains backward compatibility while improving the architecture. RecommendationApprove. This PR successfully advances the executor refactoring goals with minimal risk, good test coverage, and consistent application of the refactoring pattern across the codebase. No blocking issues were identified in the review. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ReviewThis is the PR the stack needed. 531 What I like
Concerns
VerdictApprove. This delivers on the promise of #222/#224/#225: handlers are decoupled from |
Summary
Third PR in the executor refactoring series (follows #222 typed errors, #224 dispatch registry, #225 handler migration).
Backend,MprPath,SqlMgr,ThemeRegistry,Settings,Connected()/ConnectedForWrite()methodse.reader/e.writercalls replaced withctx.Backendacross 66 files;e.quiet/e.format/e.fragments/e.mprPath/e.outputreplaced withctx.*equivalentsgetEntityNames,getMicroflowNames,getPageNamesconverted from Executor methods to free functions usingctx.Backend/ctx.Cache;getThemeRegistry→ctx.GetThemeRegistry()lazy-load methodcatalog.NewBuilderaccepts newCatalogReaderinterface instead of concrete*mpr.ReadershowEnumerationshandler tested withMockBackend, demonstrating the testability gains~27
e := ctx.executorlines remain across ~18 files for legitimate reasons: Executor method calls (trackModifiedDomainModel,Execute,ExecuteProgram), shared state mutations (catalog,cache,sqlMgr), and concrete*mpr.Reader/*mpr.Writertype requirements (pageBuilder, lint, import/export mappings).Commits (5)
Verification
make build— cleanmake test— all passmake lint— cleanDepends on: #225 (merged)