Add test enablers: MockPageMutator, MockWorkflowMutator, registry completeness

[retran]

Summary

Test infrastructure that unblocks subsequent test coverage PRs:

• MockPageMutator (16 methods) and MockWorkflowMutator (15 methods) — same Func-field delegation pattern as existing MockBackend, with compile-time interface checks
• Registry completeness fix — allKnownStatements() was missing 8 agent editor statement types that were registered but not validated
• Handler count snapshot test — catches divergence between registered handlers and known statement list

Context

Part of stacked PR series for MDL subsystem test coverage (PR A). Depends on PR #243 (fix/dset-silent-failure). Retarget base to main after #243 merges.

[github-actions]

AI Code Review

What Looks Good

• The PR adds test infrastructure (MockPageMutator, MockWorkflowMutator) following the established Func-field delegation pattern used by MockBackend
• Registry completeness fix addresses a real gap where 8 agent editor statement types were registered but not validated in allKnownStatements()
• Handler count snapshot test provides ongoing protection against registry/handler divergence
• Workflow mutator bug fix properly handles the case where TaskPage key is absent in BSON (previously dSet would silently fail)
• All changes are well-scoped to test infrastructure without affecting production logic
• Mock implementations return appropriate zero values/nil errors when functions aren't configured (never panics)
• Changes follow existing code patterns and conventions

Recommendation

Approve. This PR adds valuable test enablers that will unblock subsequent test coverage PRs, fixes an actual bug in workflow mutator handling, and maintains consistency with existing patterns in the codebase. No checklist violations apply to this test infrastructure-focused change.

---

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

[Copilot]

Pull request overview

Adds test infrastructure and strengthens registry/workflow mutation correctness checks to unblock broader MDL subsystem test coverage work.

Changes:

• Add MockPageMutator and MockWorkflowMutator test doubles that implement the corresponding backend mutator interfaces via Func-field delegation.
• Fix allKnownStatements() completeness for agent editor statement types and add a handler-count parity test vs. known statement types.
• Fix workflow PAGE mutation when TaskPage key is absent by appending the key and replacing the activity document in the BSON tree; add regression coverage (including nested sub-flow case).

Reviewed changes

Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
mdl/types/id_test.go	Test formatting/alignment tweak.
mdl/types/edmx_test.go	Test struct field alignment tweak.
mdl/executor/widget_registry.go	Minor formatting alignment in WidgetRegistry fields.
mdl/executor/registry_test.go	Expand known statement list and add handler-count parity test.
mdl/backend/mpr/workflow_mutator_test.go	Update/strengthen regression coverage for PAGE mutation (missing key + nested sub-flow).
mdl/backend/mpr/workflow_mutator.go	Ensure PAGE mutation works when TaskPage key is absent by replacing the activity in the BSON tree.
mdl/backend/mpr/convert_roundtrip_test.go	Remove trailing whitespace line.
mdl/backend/mpr/backend.go	Formatting/go-fmt style adjustments.
mdl/backend/mock/mock_workflow_mutator.go	New WorkflowMutator mock for tests.
mdl/backend/mock/mock_page_mutator.go	New PageMutator mock for tests.

---

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

[github-actions]

AI Code Review

What Looks Good

• Test infrastructure: The PR adds MockPageMutator and MockWorkflowMutator following the established MockBackend pattern with Func-field delegation and compile-time interface checks (var _ backend.PageMutator = (*MockPageMutator)(nil)). This enables safer, more maintainable testing.
• Registry completeness: Fixed allKnownStatements() to include 8 missing agent editor statement types (Create/Drop Agent, ConsumedMCPService, KnowledgeBase, Model), ensuring registry validation stays current with registered handlers.
• Handler count snapshot test: Added TestNewRegistry_HandlerCountSnapshot to proactively catch future divergences between registered handlers and known statement lists.
• Bug fix: Corrected SetActivityProperty for PAGE in workflow mutator to handle missing TaskPage keys by appending the key to the activity document (fixing silent failures where dSet would fail on absent keys).
• Test coverage: Added comprehensive tests for the workflow mutator fix, including nested sub-flow scenarios, verifying correct BSON tree mutation.
• Code quality:
  • Backend method formatting improvements (adding braces for consistency)
  • Helper function replaceActivity properly handles recursive BSON tree updates
  • All mock methods safely return nil/zero values when unset (no panics)
  • Test names clearly describe scenarios (e.g., _MissingKey_NestedSubFlow)

Recommendation

Approve - The PR successfully delivers test infrastructure that unblocks future coverage work, fixes a real bug in workflow mutation, and adds preventive measures to maintain registry consistency. All changes are focused, follow existing patterns, and include appropriate test coverage. No critical or moderate issues identified.

---

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 10 changed files in this pull request and generated no new comments.

---

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

[ako]

Review: PR #244 — Add test enablers: MockPageMutator, MockWorkflowMutator, registry completeness

Verdict: Approve with notes — solid test infrastructure. The mocks are cleanly implemented and the registry snapshot test adds real value. One convention question worth discussing.

---

Overview

Three distinct additions:

1. MockPageMutator (16 methods) and MockWorkflowMutator (15 methods) in mdl/backend/mock/ — Func-field delegation pattern with compile-time interface checks
2. allKnownStatements() patched to include 8 agent editor statement types that were registered but missing from the validation list
3. TestNewRegistry_HandlerCountSnapshot — catches the complementary failure case to TestNewRegistry_Completeness (registered handlers with no entry in allKnownStatements)

Note: this diff also includes the workflow_mutator.go fix and backend.go formatting from PR #243 (expected for a stacked PR — not re-reviewing those changes here).

---

MockPageMutator / MockWorkflowMutator

• Compile-time checks (var _ backend.PageMutator = (*MockPageMutator)(nil)) are present. ✓
• Method coverage matches the interfaces. ✓
• ContainerType() default returns backend.ContainerPage rather than the zero value — reasonable for tests, but it's the only method in either mock that doesn't return a pure zero value. Worth a comment explaining the choice.

Convention gap: CLAUDE.md specifies that mock stubs should have a descriptive error default ("MockBackend.X not configured") rather than nil, nil. Both new mocks return nil errors when the Func field is unset:

func (m *MockPageMutator) SetWidgetProperty(...) error {
    if m.SetWidgetPropertyFunc != nil {
        return m.SetWidgetPropertyFunc(...)
    }
    return nil  // ← CLAUDE.md convention says this should be a named error
}

For mutator mocks this is actually a defensible design choice — silent no-ops let test authors stub only what they care about. But it's inconsistent with the existing MockBackend pattern. If the intent is to diverge from the convention for mutators (where "not configured = no-op" is more useful than "not configured = error"), that should be called out explicitly — either in the PR description or in a comment on the struct so the next contributor knows it's intentional.

---

Registry Completeness Fix

The 8 added statement types (CreateAgentStmt, CreateConsumedMCPServiceStmt, CreateKnowledgeBaseStmt, CreateModelStmt, and their Drop* counterparts) are the agent editor statements shipped in recent PRs. Good catch — these were registered handlers but invisible to TestNewRegistry_Completeness.

---

TestNewRegistry_HandlerCountSnapshot

This test complements TestNewRegistry_Completeness in the opposite direction:

• Completeness catches: known statement has no handler
• HandlerCountSnapshot catches: registered handler has no entry in allKnownStatements()

Together they form a two-way contract. Good design.

One concern: the error message could be more actionable. The current message tells you the counts diverge but not which side has the extra entry:

t.Errorf("handler count mismatch: registry has %d, allKnownStatements has %d — update allKnownStatements or register missing handlers", got, len(known))

Since TestNewRegistry_Completeness already surfaces which known statements have no handler, the snapshot test primarily catches the reverse — a handler registered for an unknown type. The message could note this: "registry has more handlers than allKnownStatements — a handler may be registered for an unlisted statement type" when got > len(known).

---

Minor

• HandlerCount() is called in the test but not shown in the diff — presumably already defined on Registry. If it was added in this PR it should appear in the diff; worth confirming it's not silently excluded.
• The formatting-only changes to backend.go, widget_registry.go, edmx_test.go, id_test.go are cosmetic carry-overs from the stack — fine.