refactor: introduce LintReader interface, decouple linter from sdk/mpr

[retran]

Why

The linter and executor packages import sdk/mpr directly for *mpr.Reader, coupling them to the concrete SQLite implementation. This blocks the WASM build (PR #7 series) where sdk/mpr is unavailable, and prevents testing with mock backends. Introducing a LintReader interface breaks this dependency using Go's implicit interface satisfaction — zero changes needed in the 10+ linter rule files.

Summary

• Define LintReader interface in mdl/linter/context.go with 7 methods matching both *mpr.Reader and MprBackend signatures
• Replace concrete *mpr.Reader dependency with LintReader interface throughout linter and executor packages
• Remove Reader() methods and readerProvider interface from executor — callers now pass Backend() directly
• Zero sdk/mpr imports remain in mdl/linter/ and mdl/executor/ production code

Test

make build && make test && make lint-go — all pass

[github-actions]

AI Code Review

Critical Issues

None

Moderate Issues

None

Minor Issues

None

What Looks Good

• Clean decoupling: Successfully replaces concrete *mpr.Reader dependency with LintReader interface across linter and executor packages
• WASM compatibility: Removes all sdk/mpr imports from linter/executor production code as stated
• Minimal disruption: Zero changes needed in 10+ linter rule files due to implicit interface satisfaction
• Consistent refactor: Applied uniformly across:
  • Command files (cmd_lint.go, cmd_report.go)
  • Executor context (exec_context.go): Removed deprecated Reader() and readerProvider
  • Executor (executor.go): Replaced Reader() with proper Backend()/IsConnected() methods
  • Linter context (context.go): Defined LintReader interface with 7 methods matching both *mpr.Reader and MprBackend
  • Linter rule (page_navigation_security.go): Updated to use LintReader
• Backward compatibility: Maintains Reader() method on executor for backward compatibility (now delegates to Backend())
• Test validation: All build/test/lint commands pass as reported

Recommendation

Approve. The refactor cleanly achieves its goal of decoupling linter/executor from SQLite-specific code while maintaining full functionality and enabling WASM builds. The changes are focused, consistent, and follow Go interface best practices. No modifications to MDL syntax or pipeline were needed (as this is purely a backend abstraction refactor).

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• In mdl/executor/executor.go, the Reader() method now returns backend.FullBackend but the comment says it's for "backward compatibility with callers that used the former sdk/mpr.Reader accessor." However, backend.FullBackend is not equivalent to *mpr.Reader - it's a broader interface. This might break callers expecting the specific *mpr.Reader type. The PR should either:
  a) Keep the original Reader() signature returning *mpr.Reader (delegating to backend when possible), or
  b) Update all callers to use the new Backend() method instead of Reader()
• The roundtrip test update changed sdk/mpr/version to mdl/types for ProjectVersion. While this seems correct, I should verify that mdl/types.ProjectVersion has the same interface as the old version type (specifically the IsAtLeast method that was added). The diff shows the method was added, so this appears fine.

What Looks Good

• Successfully decouples linter/executor from concrete sdk/mpr implementation using Go's implicit interface satisfaction
• The LintReader interface is narrowly defined with exactly the 7 methods needed by lint rules
• Zero changes required in linter rule files as claimed
• Enables WASM builds by removing sdk/mpr imports from linter/executor production code
• Maintains backward compatibility through executor's Reader() method (though see minor issue above)
• Refactor is consistently applied across all affected files
• All tests pass as reported (make build && make test && make lint-go)
• Follows the project's backend abstraction pattern properly

Recommendation
Approve with minor clarification needed about the Reader() return type compatibility. The interface approach is sound, but the exact return type of the backward-compatible Reader() method should be verified to ensure it doesn't break existing callers that expect *mpr.Reader. If the callers have been updated to use Backend() instead (as seen in cmd_lint.go and cmd_report.go), then the backward-compatible Reader() method may not be needed or should maintain the original signature.

---

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

[retran]

Thanks for the review — no issues found, agreed on all points.

The Reader() backward-compat shim and the consistent interface approach are working as intended.

[retran]

Good catch on the Reader() return type question. Clarification:

The Reader() shim intentionally returns backend.FullBackend rather than *mpr.Reader. This is safe because:

1. No external callers use executor.Reader() with a concrete *mpr.Reader type assertion. All callers in the codebase access Reader() through the linter context interface, which only requires LintReader (7 methods). Both *mpr.Reader and backend.FullBackend satisfy this interface.

2. The only direct executor.Reader() callers were cmd_lint.go and cmd_report.go — both updated in this PR to use Backend() instead.

3. Reader() on *mpr.Writer (used in docker/build.go, examples, and backend/mpr/backend.go) is a different method on a different type — unaffected by this change.

The shim exists solely for any out-of-tree callers that might reference executor.Reader(). Returning backend.FullBackend is a strict superset, so it's backward-compatible at the interface level. If we find a caller doing a concrete type assertion to *mpr.Reader, that would already be broken by the backend abstraction and should be migrated.

Re: types.ProjectVersion.IsAtLeast — correct, the method was added in this PR to mdl/types/infrastructure.go to match the old sdk/mpr/version API.

[ako]

Code Review — PR #289

Overview: Introduces LintReader interface in mdl/linter/context.go to decouple linter and executor from *mpr.Reader. Clean, minimal diff — the 10+ rule files need no changes because Go's implicit interface satisfaction does the work. The direction is correct.

---

Good

• Zero sdk/mpr imports remain in mdl/linter/ and mdl/executor/ production code — stated goal achieved
• MockBackend already satisfies LintReader (all 7 methods present), so linter rules are immediately testable with mocks
• Moving IsAtLeast to mdl/types.ProjectVersion is the right direction — mdl/ code should use types, not sdk/mpr/version
• Constructor injection (NewLintContext(cat, reader)) is cleaner than the old two-step New + SetReader

---

Issues

1. Missing compile-time assertion for LintReader

There's no var _ linter.LintReader = (*mpr.MprBackend)(nil) check anywhere. If a future PR removes or renames one of the 7 interface methods in the backend, the breakage is silent until a linter rule calls it at runtime. Add to mdl/backend/mpr/backend.go:

var _ linter.LintReader = (*MprBackend)(nil)

2. Reader() shim is a silent API break

Executor.Reader() changes return type from *mpr.Reader to backend.FullBackend. The comment says "backward compatibility" but the signature changed — callers that stored the result as *mpr.Reader will fail to compile. This isn't backward compatible; it's a type replacement. The shim gives false confidence. Either drop it (it was already Deprecated) or keep the old signature and panic/log if someone calls it (clearer failure mode).

3. Implicit nil reader passed to NewLintContext

Previously, cmd_lint.go and cmd_report.go had an explicit nil guard before calling SetReader:

if reader := exec.Reader(); reader != nil {
    ctx.SetReader(reader)
}

Now exec.Backend() (which returns nil when not connected) is passed unconditionally to NewLintContext. Rules already need to nil-check ctx.Reader(), so this is safe in practice — but worth a comment on NewLintContext that reader may be nil and rules must guard accordingly.

4. IsAtLeast now exists on two distinct types

mdl/types.ProjectVersion.IsAtLeast (new) and sdk/mpr/version.ProjectVersion.IsAtLeast (existing) are separate implementations on separate types with the same logic. There's no deduplication plan. At minimum, add a // TODO: remove once all callers use types.ProjectVersion note to sdk/mpr/version to track the migration.

---

Minor

• roundtrip_helpers_test.go:599 calls e.executor.Reader().ProjectVersion() — this now calls through backend.FullBackend.ProjectVersion() returning *types.ProjectVersion, which has IsAtLeast from this PR. Works, but the indirection via the deprecated Reader() shim should be updated to e.executor.Backend().ProjectVersion() for clarity.
• LintReader is defined in context.go but semantically belongs closer to an interface file. Minor — fine as-is given the package size.

---

Overall: Approve with the compile-time assertion added (#1) — it's a one-liner and guards against silent future regressions. The Reader() shim issue (#2) is worth fixing before merge to avoid confusing external callers.

[retran]

Thanks for the review! All items addressed in ab43ea1:

1. Compile-time assertion — added var _ linter.LintReader = (*MprBackend)(nil) alongside existing FullBackend assertion.
2. Reader() shim — removed the deprecated method entirely. Only 2 test callers existed; updated both to use Backend().
3. Nil reader comment — added doc on NewLintContext: "reader may be nil; rules that require backend access must check Reader() != nil."
4. IsAtLeast dedup — went with the shared type approach: sdk/mpr/version.ProjectVersion is now a type alias for types.ProjectVersion. Moved IsAtLeastFull, String, IsMPRv2 to types.ProjectVersion. Converted IsSupported/SupportsFeature to package-level functions (can't add methods to type alias). Removed convertProjectVersion and its tests since the types are now identical.
5. Minor — updated test callers to use Backend().ProjectVersion().

[github-actions]

AI Code Review

What Looks Good

This PR successfully introduces the LintReader interface to decouple the linter and executor packages from the concrete sdk/mpr implementation, enabling WASM builds and mock backend testing. The changes are:

• Well-focused: Addresses a single concern (dependency decoupling) without mixing in unrelated changes
• Consistently applied: All linter and executor code now uses the LintReader interface
• Architecturally sound: Uses Go's implicit interface satisfaction correctly - zero changes needed in lint rule files
• Thoroughly tested: Author reports make build && make test && make lint-go all pass
• Clean cleanup: Removed unused conversion functions and deprecated Reader() methods
• Preserves functionality: All existing behavior maintained while improving modularity

The implementation correctly:

• Defines a minimal interface with exactly the 7 methods lint rules need
• Has MprBackend implement LintReader
• Updates all callers to pass Backend() directly instead of setting reader separately
• Moves version methods to types.ProjectVersion since it's now an alias
• Maintains proper error handling throughout

Recommendation

Approve - This is a clean, focused refactor that improves the codebase's modularity and testability without changing behavior. It successfully addresses the WASM build blocker and enables mock backend testing as intended. All checklist items are either satisfied or not applicable to this refactor.

---

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

[github-actions]

AI Code Review

What Looks Good

• Successfully introduces LintReader interface to decouple linter/executor from concrete sdk/mpr implementation
• Zero sdk/mpr imports remain in mdl/linter/ and mdl/executor/ production code as stated
• Enables WASM builds (PR GALLERY widget is completely unusable via MDL in mxcli v0.2.0-dirty #7 series) by removing the problematic dependency
• Allows testing with mock backends through interface satisfaction
• Minimal interface design with exactly 7 methods needed by lint rules
• Consistent application across all affected packages (linter, executor, cmd, backend, types)
• Test updates show proper usage of the new interface pattern
• make build && make test && make lint-go passes as reported
• No breaking changes to public API - internal refactor only

Recommendation

Approve - This refactor cleanly achieves its goal of decoupling the linter and executor packages from the SDK's concrete SQLite implementation while maintaining all existing functionality. The interface-based approach follows Go best practices and enables the requested WASM build support and improved testability. All checklist items are satisfied for this type of internal refactor.

---

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

[ako]

Code Review — PR #289 (updated review)

Overview: Decouples linter and executor from *mpr.Reader by introducing LintReader interface. This is a substantially stronger version than the original draft — all three main issues from the earlier review have been addressed.

---

What's improved since the first review

• Compile-time assertion added — var _ linter.LintReader = (*MprBackend)(nil) in mdl/backend/mpr/backend.go ✓
• Reader() shim removed entirely — Executor.Reader() is gone; callers migrated to Backend() directly ✓
• Nil reader documented — NewLintContext comment notes reader may be nil ✓
• IsAtLeast duplication eliminated — version.ProjectVersion is now a type alias for types.ProjectVersion; methods live in mdl/types once, used everywhere ✓
• convertProjectVersion eliminated — no longer needed since it's the same type ✓

---

One remaining issue

IsSupported / SupportsFeature method-to-function conversion is an undocumented public API break

The PR converts:

// before (methods)
pv.IsSupported()
pv.SupportsFeature(feature)

// after (package functions)
version.IsSupported(pv)
version.SupportsFeature(pv, feature)

A grep shows no callers outside version.go itself, so this is safe for internal code. But these are exported symbols — if anyone imports sdk/mpr/version externally, their code breaks silently. Worth a CHANGELOG entry under Changed (analogous to the mdl/types extract entry in v0.7.0).

---

Minor observations

• MockBackend doesn't have var _ linter.LintReader = (*MockBackend)(nil) — not a safety gap since MockBackend already has var _ backend.FullBackend and FullBackend is a superset of LintReader. But adding it would make the mock's linting capability explicit and keep the two assertions in sync.
• mdl/linter now imports sdk/microflows, sdk/pages, sdk/security for the interface method signatures. This is unavoidable and correct — no new cycles introduced.

---

Overall: Approve. The type-alias approach for ProjectVersion is particularly clean. The IsSupported/SupportsFeature change is the only thing worth noting before merge, and only as a changelog entry since there are no internal callers to update.