Review followup: PR #825 — GH#820: backfill postmeta for nav_menu_item, attachment, and Elementor posts on site clone

[superdav42]

Unaddressed review bot suggestions

PR #825 was merged with unaddressed review bot feedback. Each comment
below includes its file path, line number, a direct link to the inline
review comment, and a diff fence with the code context the bot was
flagging. Resolved and outdated threads are filtered out via GitHub's
GraphQL review-thread state. Read the relevant lines, decide whether
the suggestion is correct, and either apply the fix or close this issue
with a wontfix rationale.

Source PR: #825

---

You are the triager (worker-is-triager rule)

This issue is auto-created from review bot output and dispatched
directly to you. Review bots can be wrong: hallucinated line refs, false
premises about codebase structure, template-driven sweeps without
measurements (see GH#17832-17835 for prior art and prompts/build.txt
section 6a). Do not assume the bot is correct. Verify before acting.

You must end in exactly one of three outcomes — no fourth "hand it back
to the human" path exists. Humans approve decisions; they do not re-do
analysis.

Outcome A — Premise falsified → close the issue

1. Read the cited file:line (listed under Files to modify below).

2. If the bot's claim is factually wrong (file doesn't exist at that
   line, function doesn't behave as described, "auto-generated" section
   isn't actually auto-generated, etc.), close the issue with a
   comment in this shape:

   Premise falsified. <what the bot claimed>. <what the code
   actually shows, with a file:line citation or one-line quote>.
   Not acting.

   No PR. No further dispatch. The closing comment trains the next
   session reading this thread and the noise filter.

Outcome B — Premise correct + fix is obvious → implement and PR

1. Verify the bot's premise as above.
2. Read the Worker Guidance section below, open a worktree, implement.
3. Open a PR with Resolves #<this-issue-number> in the body
   (use THIS issue's number, not the source PR's) so merge auto-closes it.
4. Follow the normal Lifecycle Gate (brief, tests, review-bot-gate,
   merge, postflight).

Outcome C — Premise correct but approach is a genuine judgment call

Only use this path if you reach it after Outcomes A and B don't apply:
the bot's finding is real, but the fix requires a decision that is
architectural, policy, breaking-change, or otherwise genuinely outside
what you can resolve autonomously. In that case, post a decision
comment with exactly these fields:

• Premise check: one line, confirming the finding is real.
• Analysis: 2-4 bullets on the trade-offs.
• Recommended path: the option you would take if the decision were
  yours, with rationale.
• Specific question: the single decision the human needs to make
  (yes/no or pick-one, not open-ended).

Then apply needs-maintainer-review and stop. The human wakes up to a
ready-to-approve recommendation, not a blank task.

Ambiguity about scope or style is not Outcome C. Per
prompts/build.txt "Reasoning responsibility", the model does the
thinking and delivers a recommendation. Only escalate what is genuinely
a maintainer-only decision.

Worker Guidance

Files to modify:

• inc/helpers/class-site-duplicator.php:269
• inc/helpers/class-site-duplicator.php:296
• tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php:615

Implementation steps (Outcome B path):

1. Read the diff block under each inline comment below — it shows the
   exact code the bot was flagging. Open the file only if you need
   surrounding context beyond what the diff tail shows.
2. Read the bot's full comment below the diff — it contains the rationale
   and any suggested change.
3. Verify the premise before implementing (see Outcome A). If the premise
   is wrong, switch to Outcome A instead of burning iterations trying to
   satisfy a wrong suggestion.
4. If multiple comments target the same file, group your edits into one
   logical commit.
5. Run shellcheck / markdownlint-cli2 / project tests as appropriate.

Verification:

• Open the new PR with Resolves #<this-issue> so this followup is auto-closed on merge.
• If the bot's suggestion was incorrect, close this issue with a Outcome A comment — do not open a no-op PR.

Inline comments

coderabbitai on inc/helpers/class-site-duplicator.php:269

View inline comment

+		 *
+		 * MUCD's hooks pass a from_site_id that may differ from the template
+		 * the customer actually selected at checkout. WP Ultimo stores the
+		 * customer's real choice in the wu_template_id site meta key.
+		 * Prefer that over the explicit param when available.
+		 *
+		 * @since 2.3.1
+		 * @see https://github.com/Ultimate-Multisite/ultimate-multisite/issues/820
+		 */
+		$meta_template = (int) get_site_meta($args->to_site_id, 'wu_template_id', true);
+		if ($meta_template > 0 && $meta_template !== (int) $args->from_site_id) {
+			$args->from_site_id = $meta_template;

🛠️ Refactor suggestion | 🟠 Major

Don't overwrite from_site_id in place after copy_data().

copy_data() and copy_files() have already run with the original source by this point, but copy_users() and the wu_duplicate_site payload now read the rewritten value. If wu_template_id differs, the clone can mix tables/files from one site with users and downstream hook context from another. Keep the resolved template ID in a separate variable for the backfill/hook path instead of mutating $args->from_site_id.

💡 Proposed direction

-		$meta_template = (int) get_site_meta($args->to_site_id, 'wu_template_id', true);
-		if ($meta_template > 0 && $meta_template !== (int) $args->from_site_id) {
-			$args->from_site_id = $meta_template;
-		}
+		$template_site_id = (int) $args->from_site_id;
+		$meta_template    = (int) get_site_meta($args->to_site_id, 'wu_template_id', true);
+		if (0 < $meta_template && $meta_template !== (int) $args->from_site_id) {
+			$template_site_id = $meta_template;
+		}
…
-		self::backfill_postmeta($args->from_site_id, $args->to_site_id);
-		self::verify_kit_integrity($args->from_site_id, $args->to_site_id);
+		self::backfill_postmeta($template_site_id, $args->to_site_id);
+		self::verify_kit_integrity($template_site_id, $args->to_site_id);
…
-				'from_site_id' => $args->from_site_id,
+				'from_site_id' => $template_site_id,

Also applies to: 298-320

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@inc/helpers/class-site-duplicator.php` around lines 267 - 269, Do not mutate
$args->from_site_id after copy_data()/copy_files() have run; instead resolve the
template override into a new variable (e.g. $resolved_template_from_site_id or
$template_source_id) when you read wu_template_id and, for the backfill/hook
path and copy_users()/wu_duplicate_site payload only, use that new variable;
leave $arg

coderabbitai on inc/helpers/class-site-duplicator.php:296

View inline comment

+
+		/*
+		 * Verify Kit integrity after backfill.
+		 *
+		 * Compares the byte length of _elementor_page_settings between the
+		 * template and the clone. If the clone has less than 80% of the
+		 * template's byte count, the Kit fix is re-applied as a safety net.
+		 *
+		 * @since 2.3.1
+		 * @see https://github.com/Ultimate-Multisite/ultimate-multisite/issues/820
+		 */
+		self::verify_kit_integrity($args->from_site_id, $args->to_site_id);

⚠️ Potential issue | 🟠 Major

Backfilled postmeta is bypassing MUCD's rewrite step.

These copies happen after MUCD_Data::copy_data() has already finished db_update_data() in inc/duplication/data.php:26-150. Any source-domain values embedded in _elementor_*, Kit settings, or custom menu URLs are inserted verbatim here, so they won't go through the same source→target replacement pass as the rest of the clone.

Also applies to: 381-385, 424-586

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@inc/helpers/class-site-duplicator.php` around lines 284 - 296, The
backfill_postmeta call is copying raw _elementor_* and other postmeta after
MUCD_Data::copy_data() (which runs db_update_data()) finishes, so those values
never pass through the source→target rewrite; move or re-run the rewrite for
these meta values by either (A) performing the backfill_postmeta step before
MUCD_Data::copy_data()/db_update_data() or (B) invoking the same rewrite/replace
routine that db_update_data() uses on the newly-inserted meta after
self::backfill_postmeta(...) completes; locate and update the call to
self::backfill_postmeta and ensure you apply the replacement logic to keys like
_elementor_page_settings, other _elementor_* keys and menu URLs (and keep
verify_kit_integrity after the rewrite) so cloned meta contain target-domain
values.

coderabbitai on tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php:615

View inline comment

+		$result = Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site', $args);
+
+		if ( ! is_wp_error($result)) {
+			$this->assertIsArray($captured);
+			$this->assertArrayHasKey('from_site_id', $captured);
+			$this->assertArrayHasKey('site_id', $captured);
+			$this->assertEquals($this->template_site_id, $captured['from_site_id']);
+			$this->assertEquals($result, $captured['site_id']);
+
+			wpmu_delete_blog($result, true);
+		}
+	}

⚠️ Potential issue | 🟡 Minor

Make this test fail when it can't reach the payload assertions.

If duplicate_site() returns WP_Error, this test silently skips every assertion about wu_duplicate_site. That turns an environment or duplication failure into a false positive for the new contract.

✅ Tighten the assertion path

 		$result = Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site', $args);
-
-		if ( ! is_wp_error($result)) {
-			$this->assertIsArray($captured);
-			$this->assertArrayHasKey('from_site_id', $captured);
-			$this->assertArrayHasKey('site_id', $captured);
-			$this->assertEquals($this->template_site_id, $captured['from_site_id']);
-			$this->assertEquals($result, $captured['site_id']);
-
-			wpmu_delete_blog($result, true);
-		}
+		if (is_wp_error($result)) {
+			$this->fail('Site duplication failed: ' . $result->get_error_message());
+		}
+
+		$this->assertIsArray($captured);
+		$this->assertArrayHasKey('from_site_id', $captured);
+		$this->assertArrayHasKey('site_id', $captured);
+		$this->assertEquals($this->template_site_id, $captured['from_site_id']);
+		$this->assertEquals($result, $captured['site_id']);
+
+		wpmu_delete_blog($result, true);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php` around lines 604 - 615,
Change the test so it fails immediately when Site_Duplicator::duplicate_site
returns a WP_Error instead of silently skipping assertions: after calling
Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site',
$args) check the result with an assertion that it is not a WP_Error (e.g.
assertFalse(is_wp_error($result)) or assertNotInstanceOf(WP_Error::class,
$result)) and include the WP_Error message for context; then proceed to assert
against $captured and call wpmu_delete_blog($result, true) on

PR review summaries

(none)

---

aidevops.sh v3.8.22 with claude-sonnet-4-6 spent 1h 38m and 8 tokens on this as a headless worker.

[superdav42]

DISPATCH_CLAIM nonce=b66413462007015ec3606c3c50a54a47 runner=superdav42 ts=2026-04-14T04:06:32Z max_age_s=1800

[superdav42]

Dispatching worker (deterministic).

• Worker PID: 2173213
• Model: anthropic/claude-sonnet-4-6
• Tier: standard
• Runner: superdav42
• Issue: Review followup: PR #825 — GH#820: backfill postmeta for nav_menu_item, attachment, and Elementor posts on site clone #834

[superdav42]

DISPATCH_CLAIM nonce=da4b13a0d4bf07515f681d2fc4ebdd85 runner=superdav42 ts=2026-04-14T04:38:00Z max_age_s=1800