fix: handle missing TaskPage key in workflow activity mutation

[retran]

Summary

When setting PAGE on a workflow activity where the TaskPage BSON key is absent (not just nil), dSet silently failed because it only updates existing keys — the page reference was lost.

This adds a replaceActivity helper that appends the missing key to the activity document and replaces it in the workflow's BSON tree by $ID match.

Three cases now handled correctly:

• TaskPage exists with value: update Page field in place (unchanged)
• TaskPage key exists with nil value: replace via dSet (unchanged)
• TaskPage key absent: append key, replace activity in BSON tree (new)

The existing regression test (TestWorkflowMutator_SetActivityProperty_Page_MissingKey) now asserts correct behavior instead of documenting the bug.

[github-actions]

AI Code Review

What Looks Good

• The fix correctly addresses the core issue: dSet only updates existing keys, so when TaskPage key is absent, the page reference was lost
• The solution handles all three cases properly:
  1. TaskPage exists with value: update Page field in place (unchanged behavior)
  2. TaskPage key exists with nil value: replace via dSet (unchanged behavior)
  3. TaskPage key absent: append key and replace activity in BSON tree (new correct behavior)
• The helper function replaceActivity is well-named, documented, and encapsulates the complex BSON tree replacement logic
• The test now correctly asserts the expected behavior instead of just documenting the bug
• The recursive traversal properly handles nested workflows by checking for nested flows in activities
• No breaking changes to existing functionality

Recommendation

Approve. This is a minimal, targeted bug fix that resolves the silent failure when setting PAGE on workflow activities with missing TaskPage keys. The change follows existing patterns, updates tests appropriately, and maintains backward compatibility. No checklist violations apply as this is a backend-only fix without MDL syntax changes.

---

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

[Copilot]

Pull request overview

Fixes a workflow mutation edge case where setting the PAGE property on an activity silently failed when the TaskPage BSON key was entirely absent (not present with nil).

Changes:

• Updates SetActivityProperty(..., "PAGE", ...) to handle the “missing key” case by appending TaskPage and reattaching the updated activity into the workflow BSON tree.
• Adds replaceActivity / replaceActivityRecursive helpers to replace an activity in Flow.Activities (including nested flows) by matching $ID.
• Updates the existing regression test to assert correct behavior when TaskPage is missing.

Reviewed changes

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

File	Description
mdl/backend/mpr/workflow_mutator.go	Adds recursive activity replacement helper and uses it when TaskPage key is absent during PAGE mutation.
mdl/backend/mpr/workflow_mutator_test.go	Converts the prior “documents the bug” test into a true regression assertion for the missing-key case.

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• None found

What Looks Good

• Clear problem identification: The PR correctly identifies that dSet silently fails when trying to set a value on a BSON key that doesn't exist at all (not just nil)
• Comprehensive solution: Handles all three cases properly:
  1. TaskPage exists with value: update in place (existing behavior preserved)
  2. TaskPage key exists with nil value: replace via dSet (existing behavior preserved)
  3. TaskPage key absent: append key and replace activity in BSON tree (new correct behavior)
• Good test coverage:
  • Updates existing regression test to verify correct behavior instead of just documenting the bug
  • Adds test for nested sub-flow scenario to ensure recursive replacement works
  • Tests cover the actual integration path, not just helpers
• Clean implementation:
  • Extracts replaceActivity helper for clarity and reuse
  • Properly handles activity replacement by $ID match in BSON tree
  • Includes recursive handling for nested activities in sub-flows
  • Early returns when IDs can't be extracted (defensive programming)
• Scoped correctly: Focuses solely on fixing the missing TaskPage key issue without unrelated changes
• Follows existing patterns: Uses same BSON manipulation patterns as rest of codebase (dGet, dSet, bson.D, etc.)

Recommendation

Approve - This is a well-scoped bug fix that properly addresses the root cause with comprehensive test coverage. The implementation is clean, follows existing code patterns, and handles edge cases including nested activities. No checklist violations found.

---

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

[Copilot]

Pull request overview

Fixes workflow activity PAGE mutation so TaskPage is correctly set even when the TaskPage key is entirely absent from the activity BSON (not just present with a nil value), by replacing the updated activity back into the workflow tree by $ID.

Changes:

• Update SetActivityProperty(PAGE) to handle missing TaskPage keys by appending the field and replacing the activity in the workflow BSON tree.
• Add/adjust regression tests to assert TaskPage.Page is set for both missing-key and nested-subflow cases.
• Minor formatting/whitespace cleanup in several Go files/tests.

Reviewed changes

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

Show a summary per file

File	Description
mdl/backend/mpr/workflow_mutator.go	Adds replaceActivity / recursive traversal to replace an updated activity by $ID when appending new keys.
mdl/backend/mpr/workflow_mutator_test.go	Updates regression assertion for missing TaskPage key and adds a nested subflow test case.
mdl/backend/mpr/backend.go	Formatting-only changes to method bodies/signatures.
mdl/backend/mpr/convert_roundtrip_test.go	Removes trailing whitespace line at EOF.
mdl/executor/widget_registry.go	Formatting-only alignment changes to struct fields.
mdl/types/edmx_test.go	Formatting-only alignment changes in test struct definition.
mdl/types/id_test.go	Formatting-only alignment change in test case list.

---

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

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Formatting noise: The changes in backend.go and convert_roundtrip_test.go appear to be purely formatting adjustments (brace positioning, blank lines) that may be noise from auto-formatting. While harmless, they don't contribute to the fix and could be omitted to keep the diff focused.
• Variable naming: In workflow_mutator_test.go, the variable actID in replaceActivity is clear, but act in the outer scope might be confused with activity documents vs. activity references. However, this is minor and doesn't affect correctness.

What Looks Good

• Targeted fix: The PR precisely addresses the reported bug where dSet silently fails when the TaskPage key is absent in workflow activity BSON.
• Three-case handling: Correctly distinguishes between:
  1. TaskPage exists with value (in-place update)
  2. TaskPage key exists with nil value (existing dSet behavior)
  3. TaskPage key absent (new logic: append key + replace activity)
• Robust replacement logic: The replaceActivity helper properly handles:
  • Recursive traversal of nested flows (sub-flows, outcomes, etc.)
  • Type markers in BSON arrays (skipping int32/int activity count prefixes)
  • Safe $ID extraction with fallbacks
• Test improvements:
  • Existing regression test now asserts correct behavior instead of documenting the bug
  • Added nested sub-flow test covering recursive replacement path
  • Tests verify both the presence of TaskPage and correct Page value
• No syntax changes: As a pure bug fix, no MDL grammar/AST/executor/LSP changes were needed (correctly avoiding unnecessary full-stack wiring).

Recommendation

Approve. The fix is correct, well-tested, and maintains backward compatibility. Minor formatting noise doesn't warrant blocking merge. The PR successfully resolves the silent failure when setting PAGE on workflow activities with missing TaskPage keys.

---

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

[Copilot]

Pull request overview

Fixes workflow activity PAGE mutation to correctly set TaskPage.Page even when the TaskPage BSON key is entirely absent (not just present with nil), by appending the missing key and replacing the activity in the workflow BSON tree via $ID match.

Changes:

• Add replaceActivity / replaceActivityRecursive to support replacing an updated activity document back into the workflow BSON tree (including nested sub-flows).
• Update/strengthen workflow mutator tests to assert the fixed behavior for missing TaskPage (including a nested sub-flow regression test).
• Apply minor formatting/gofmt cleanup in a few files.

Reviewed changes

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

Show a summary per file

File	Description
mdl/types/id_test.go	Minor formatting alignment in test table.
mdl/types/edmx_test.go	Minor struct field alignment in test definitions.
mdl/executor/widget_registry.go	Minor field alignment formatting.
mdl/backend/mpr/workflow_mutator_test.go	Update regression test to assert fixed behavior; add nested sub-flow coverage for recursive replacement.
mdl/backend/mpr/workflow_mutator.go	Implement missing-key handling for PAGE by appending TaskPage and replacing the activity by $ID (including nested flows).
mdl/backend/mpr/convert_roundtrip_test.go	Remove trailing blank line.
mdl/backend/mpr/backend.go	Formatting/gofmt changes for method layout.

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• Code formatting: In mdl/backend/mpr/backend.go, the brace style changes (adding braces for single-line functions) are inconsistent with the surrounding code style in the same file (which previously used braces for multi-line functions only). While not incorrect, it creates minor noise. However, this is a trivial style change and doesn't affect functionality.
• Test clarity: The new nested sub-flow test (TestWorkflowMutator_SetActivityProperty_Page_MissingKey_NestedSubFlow) uses hardcoded binary IDs with specific byte patterns. While functional, adding a comment explaining why distinct IDs are needed (to prevent false matches in replaceActivity) would improve maintainability.

What Looks Good

• Bug fix correctness: The solution properly handles all three TaskPage states (present/value, present/nil, absent) by using dSet for existing keys and a tree-replacement strategy for missing keys. The recursive traversal in replaceActivityRecursive correctly handles nested workflows.
• Test improvements:
  • The existing regression test now asserts correct behavior instead of documenting the bug.
  • The new nested sub-flow test exercises the recursive code path, ensuring correctness in complex workflow structures.
  • Tests avoid time.Sleep and use direct BSON verification.
• Code quality:
  • The replaceActivity helper is well-encapsulated and only exposed where needed.
  • Comments clearly explain why the helper is necessary (slice header propagation limitations).
  • No sdk/mpr write imports appear in the executor layer (backend abstraction compliance maintained).
  • Changes are atomic and scoped to a single concern (workflow activity mutation).

Recommendation

Approve the PR. The fix correctly resolves the reported issue with appropriate test coverage and maintains architectural boundaries. Minor style inconsistencies are trivial and do not warrant blocking merge. The nested sub-flow test could benefit from a clarifying comment but is functionally sound. All checklist items pass: no syntax changes (so MDL/full-stack sections N/A), no duplication, proper test coverage, and scope is strictly focused on the bug fix.

---

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

[Copilot]

Pull request overview

Fixes workflow activity PAGE mutation so it correctly sets TaskPage even when the TaskPage BSON key is completely absent (not just present with nil), by appending the missing key and replacing the activity document in the workflow tree by $ID.

Changes:

• Update SetActivityProperty(..., "PAGE", ...) to handle the “TaskPage key absent” case via append + activity replacement.
• Add replaceActivity / replaceActivityRecursive helper to replace an activity in nested flows by $ID.
• Strengthen regression tests to assert correct behavior for missing TaskPage, including a nested sub-flow scenario; plus minor formatting cleanups elsewhere.

Reviewed changes

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

Show a summary per file

File	Description
mdl/backend/mpr/workflow_mutator.go	Implements missing-TaskPage handling and adds activity replacement helper for appended keys.
mdl/backend/mpr/workflow_mutator_test.go	Updates regression expectations and adds nested sub-flow coverage for the recursive replacement path.
mdl/backend/mpr/backend.go	Formatting-only changes (multi-line method bodies).
mdl/backend/mpr/convert_roundtrip_test.go	Removes trailing whitespace/blank line.
mdl/executor/widget_registry.go	Formatting-only alignment changes.
mdl/types/id_test.go	Formatting-only alignment change in test table.
mdl/types/edmx_test.go	Formatting-only alignment change in test struct fields.

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• The PR includes unrelated formatting changes in multiple files (backend.go, convert_roundtrip_test.go, widget_registry.go, edmx_test.go, id_test.go). While harmless, these should be in separate commits or PRs to maintain atomicity. However, they are trivial whitespace/brace adjustments that don't affect functionality.

What Looks Good

• The fix correctly handles all three cases for TaskPage mutation (existing value, nil value, missing key)
• Added comprehensive test coverage including nested sub-flow scenario
• The replaceActivity helper properly addresses the BSON tree mutation limitation
• Clear comments explain the three handling cases
• Test now asserts correct behavior instead of documenting the bug
• No MDL syntax changes required (pure backend fix)
• Follows existing code patterns in the mutator

Recommendation
Approve the PR. The minor formatting changes are negligible and the core fix is correct, well-tested, and addresses the reported issue completely. The PR maintains proper scope by focusing on the workflow activity mutation bug.

---

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

[Copilot]

Pull request overview

Fixes workflow activity PAGE mutation so TaskPage is set correctly even when the TaskPage key is entirely absent from an activity BSON document (not just present with a nil value). This prevents silently-lost page references when mutating newly-created / incomplete activity shapes.

Changes:

• Update SetActivityProperty(PAGE) to append a missing TaskPage key and replace the activity in the workflow BSON tree by $ID.
• Add recursive replaceActivity logic to support nested sub-flow activity replacement.
• Strengthen/extend regression tests (including a nested sub-flow case) and apply minor formatting cleanups.

Reviewed changes

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

Show a summary per file

File	Description
mdl/backend/mpr/workflow_mutator.go	Implements missing-TaskPage handling and adds recursive activity replacement by $ID.
mdl/backend/mpr/workflow_mutator_test.go	Updates regression test expectations and adds nested sub-flow coverage for the replacement path.
mdl/backend/mpr/backend.go	Formatting-only changes (multi-line method bodies).
mdl/backend/mpr/convert_roundtrip_test.go	Removes trailing blank line (formatting).
mdl/executor/widget_registry.go	Field alignment/formatting.
mdl/types/edmx_test.go	Test struct field alignment/formatting.
mdl/types/id_test.go	Test case alignment/formatting.

---

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

[ako]

Review: PR #243 — fix: handle missing TaskPage key in workflow activity mutation

Verdict: Approve with suggestions — clean, well-scoped bug fix with solid test coverage. The root cause is correctly diagnosed and the fix is the right approach.

---

Overview

When ALTER WORKFLOW ... SET PAGE is called on a UserTask activity where the TaskPage key is completely absent from the BSON document (not just nil), dSet silently returned false and the page reference was lost. The fix appends the missing key and uses a new replaceActivity helper to propagate the change back into the parent BSON tree — necessary because slice appends in Go don't propagate through the existing findActivityByCaption reference.

---

Code Quality

workflow_mutator.go

• The three-case logic (key with value / key with nil / key absent) is now clearly separated and the early return nil after the in-place update removes the old ambiguous else branch. Good.
• replaceActivityRecursive correctly recurses through getNestedFlows — and importantly, it operates on elements[i] = updated which modifies the bson.A slice in place. This only works if dGetArrayElements returns the underlying bson.A without copying. Worth a comment confirming this assumption, since the correctness of the whole fix depends on it.
• replaceActivity is a package-level responsibility (not exported), which is appropriate. But it silently succeeds or fails with no signal back to the caller:

if !dSet(actDoc, "TaskPage", pageRef) {
    actDoc = append(actDoc, bson.E{Key: "TaskPage", Value: pageRef})
    m.replaceActivity(actDoc)   // ← no return value; silent on failure
}

If $ID extraction returns "" or the tree walk misses the activity (e.g., getNestedFlows doesn't cover some activity type), the PAGE reference is still silently lost. Consider returning bool from replaceActivity and logging a warning on failure — same pattern as the dSet return value already used here.

• replaceActivityRecursive is a package-level function (not a method). Minor: it could be a method on mprWorkflowMutator for consistency, but this is a style nit.

---

Test Coverage

• TestWorkflowMutator_SetActivityProperty_Page_MissingKey — correctly converted from bug-documenting to regression test. ✓
• TestWorkflowMutator_SetActivityProperty_Page_MissingKey_NestedSubFlow — exercises the recursive path, good addition. One observation: the flow/outcome nodes use make([]byte, 16) (all-zero IDs), which are distinct from parentID ({1,0,...}) and nestedID ({2,0,...}), so there's no accidental ID collision. However the test could be clearer about this — a brief comment explaining why zero-byte IDs for intermediate nodes are safe would help future readers.
• The existing TestWorkflowMutator_SetActivityProperty_Page_New (nil-value key) is preserved, confirming that path is untouched. ✓

---

Formatting / Unrelated Changes

The backend.go, widget_registry.go, edmx_test.go, and id_test.go changes are pure alignment/gofmt cleanups — no logic changes. These are fine, though per CLAUDE.md checklist, unrelated refactors ideally go in a separate commit. Low-priority given the scope is small.

---

Suggestions

1. Confirm dGetArrayElements aliasing: Add a comment on replaceActivityRecursive noting that elements must alias the underlying bson.A for in-place mutation to work. If this assumption ever breaks (e.g., if dGetArrayElements is changed to return a copy), the fix silently regresses.

2. Surface replaceActivity failures: Change replaceActivity to return bool and log a warning when the activity can't be found:

   if !m.replaceActivity(actDoc) {
       // log: "replaceActivity: activity %s not found in BSON tree"
   }

3. Test comment: In TestWorkflowMutator_SetActivityProperty_Page_MissingKey_NestedSubFlow, add a brief note that intermediate nodes use zero-byte IDs intentionally (distinct from the target IDs) to make the test self-explanatory.