rojo/.cursor/plans/matching_algorithm_parity_01524d4c.plan.md

name	overview	todos	isProject
matching algorithm parity	Fix scoring parity between the 3 matching algorithm implementations (Rust forward sync, Rust syncback, Lua plugin) and add parity/round-trip tests.	id content status fix-1-ref-scoring Fix 1: Add Ref-target-aware scoring to Rust countOwnDiffs in both forward sync and syncback matching completed id content status fix-2-tags-attrs Fix 2: Add granular Tags/Attributes scoring to Rust countOwnDiffs in both implementations completed id content status fix-3-epsilon Fix 3: Replace float_cmp::approx_eq! with custom fuzzy_eq matching Lua's 0.0001 epsilon in variant_eq.rs completed id content status fix-4-sort Fix 4: Add matched_indices sort to forward sync build_result; remove TODO from syncback completed id content status fix-5-vcache Fix 5: Cache VCache at session level in Lua matching.lua to avoid redundant decoding completed id content status fix-6-tests Fix 6: Add parity tests (Rust forward vs syncback), round-trip matching test, and Lua parity spec test completed id content status run-ci Run /ci after all fixes to verify CI passes completed	false

Matching Algorithm Parity Audit Fixes

This plan was generated by /audit from a Plan-mode session. Refer to .cursor/rules/atlas.mdc for full project standards.

Standards

These standards govern every fix in this plan. An implementer MUST read .cursor/rules/atlas.mdc before starting.

Round-Trip Identity (from atlas.mdc Quality Standard)

Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a bit-identical instance tree -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.

CLI Syncback Parity (from atlas.mdc Two-Way Sync Strategy)

Plugin-based sync must produce exactly the same filesystem output that atlas syncback would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise.

Code Quality (from atlas.mdc Code Quality Standard)

DRY code that is easy to reason about and hard to get wrong. The same slugify/dedup/meta-update pattern should not be copy-pasted across files. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred.

Context

The matching algorithm was audited across all 3 parallel implementations:

• src/snapshot/matching.rs (Rust forward sync)
• src/syncback/matching.rs (Rust syncback)
• plugin/src/Reconciler/matching.lua (Lua plugin)

The audit found scoring parity issues that could cause different implementations to produce different pairings for the same logical input. The most critical is Ref property scoring (UNMATCHED_PENALTY in Lua vs +1 in Rust), followed by Tags/Attributes granularity and float epsilon differences.

Fixes

Fix 1: Add Ref-Target-Aware Scoring to Rust countOwnDiffs

• Status: Approved
• Finding: Step 11l: Matching Algorithm Parity -- Ref property scoring
• Files: src/snapshot/matching.rs, src/syncback/matching.rs
• Problem: Lua countOwnDiffs (matching.lua lines 309-325) scores Ref property mismatches at UNMATCHED_PENALTY = 10000 by comparing target Name+ClassName. Rust count_own_diffs in both files treats Ref properties as regular Variant properties scoring +1 per mismatch. In syncback matching, Ref IDs are always different between WeakDom instances, so ALL Ref properties score +1 regardless of logical equivalence.
• Solution:
  • In forward sync count_own_diffs (matching.rs line 452-496): When comparing properties, detect Variant::Ref values. For each Ref property:
    • If both sides have a Ref value, resolve both Refs to their target instances (via tree.get_instance(ref) for tree side, look up snapshot children by snapshot_id for snap side). Compare target Name+ClassName. If different, add UNMATCHED_PENALTY instead of 1.
    • If one side has Ref and other doesn't (or Ref::none vs valid ref), add UNMATCHED_PENALTY.
    • Skip Variant::Ref from regular property comparison to avoid double-counting.
  • In syncback count_own_diffs (matching.rs line 501-548): Same approach using new_dom.get_by_ref(ref) and old_dom.get_by_ref(ref) to resolve targets.
  • Both functions need access to the DOM/tree for Ref resolution -- add the necessary parameters.
• Risk: Adding DOM access to count_own_diffs increases its parameter count. The function is #[inline], so ensure the extra parameter doesn't cause performance regression. The Ref resolution is a simple hash lookup (O(1)).
• Verify round-trip: Create 3 Models named "Weapon" with different PrimaryPart targets. Syncback, then forward sync. Verify all 3 match to their correct counterparts.
• Verify syncback parity: Compare CLI syncback output with two-way sync output for a tree with Ref-differentiated instances.
• Tests required:
  • Rust unit test: ref_scoring_forward_sync -- 2 Models with different PrimaryPart targets, verify correct pairing
  • Rust unit test: ref_scoring_syncback -- same scenario for syncback matching
  • Lua spec test: already has ref scoring test in matching.spec.lua, verify it passes

Fix 2: Add Granular Tags/Attributes Scoring to Rust countOwnDiffs

• Status: Approved
• Finding: Step 11l: Matching Algorithm Parity -- Tags/Attributes granularity
• Files: src/snapshot/matching.rs, src/syncback/matching.rs
• Problem: Lua countOwnDiffs counts per-tag (+1 each in symmetric difference) and per-attribute (+1 each diff). Rust counts Tags as one Variant property (+1 total) and Attributes as one Variant property (+1 total). An instance with 5 extra tags scores +5 in Lua but +1 in Rust.
• Solution:
  • In both Rust count_own_diffs functions:
    • When encountering Variant::Tags(tags), extract and compare granularly: +1 per tag in symmetric difference. Skip from regular property comparison.
    • When encountering Variant::Attributes(attrs), extract and compare granularly: +1 per attribute whose value differs (using variant_eq), +1 per attribute present only on one side. Skip from regular property comparison.
  • The key check is key == "Tags" (for Variant::Tags) and key == "Attributes" (for Variant::Attributes).
• Risk: Low. Only changes the cost value for Tags/Attributes, making it more discriminating. Existing tests should still pass since they don't depend on exact cost values, only on correct pairings.
• Verify round-trip: Instances differing by tags should be paired correctly.
• Verify syncback parity: N/A -- Tags/Attributes rarely distinguish instances in typical syncback scenarios.
• Tests required:
  • Rust unit test: tags_granular_scoring -- 2 Parts with different tag sets, verify scoring matches Lua behavior
  • Rust unit test: attributes_granular_scoring -- 2 Parts with different attribute maps, verify scoring

Fix 3: Loosen Rust Float Epsilon to Match Lua

• Status: Approved (Modified: loosen Rust to match Lua)
• Finding: Step 11l: Matching Algorithm Parity -- Float epsilon
• Files: src/variant_eq.rs
• Problem: Lua trueEquals uses epsilon=0.0001 with absolute+relative check. Rust variant_eq uses float_cmp::approx_eq! defaults (epsilon~1.19e-7, ulps=4), which is ~1000x tighter. This means Rust considers values different that Lua considers equal, potentially causing different match scores and different pairings.
• Solution:
  • Replace the approx_eq! macro wrapper in variant_eq.rs with a custom fuzzy_eq function that matches Lua's dual epsilon approach:

    const EPSILON: f32 = 0.0001;
    fn fuzzy_eq_f32(a: f32, b: f32) -> bool {
        if a.is_nan() && b.is_nan() { return true; }
        let diff = (a - b).abs();
        let max_val = a.abs().max(b.abs()).max(1.0);
        diff < EPSILON || diff < max_val * EPSILON
    }
    

• Apply similar fuzzy_eq_f64 for f64 comparisons.
• Update all float comparison call sites in variant_eq (Color3, CFrame, Vector3, UDim, etc.) to use the new function instead of approx_eq!.
• Remove the float-cmp dependency from Cargo.toml if no other code uses it.
• Risk: Medium. Loosening epsilon means values that were previously considered different are now equal. This affects:
  • variant_eq used in matching (intended -- this fix)
  • variant_eq used in patch_compute.rs for property diff detection (side effect -- could suppress legitimate property patches for near-equal values)
  • Verify that the 0.0001 epsilon doesn't suppress meaningful Studio property changes.
• Verify round-trip: Float properties should round-trip without phantom diffs (the looser epsilon should REDUCE phantom diffs, not create them).
• Verify syncback parity: N/A
• Tests required:
  • Rust unit test: update existing variant_eq float tests to verify the new epsilon behavior
  • Rust unit test: fuzzy_eq_matches_lua -- verify same boundary values as trueEquals.spec.lua (0.0001 threshold, relative scaling, NaN handling)

Fix 4: Add Sort to Forward Sync build_result for Consistency

• Status: Approved
• Finding: Code quality -- inconsistent sort between implementations
• Files: src/snapshot/matching.rs, src/syncback/matching.rs
• Problem: Syncback build_result (line 217) sorts matched_indices by new_ref index. Forward sync build_result (line 206) does NOT sort. The Lua matchChildren returns matched pairs in insertion order (which is group iteration order, then sorted within groups).
• Solution:
  • Add matched_indices.sort_by_key(|&(si, _)| si); to the forward sync build_result function (line 203, before the matched Vec construction).
  • Remove the TODO comment from the syncback version (line 217): the sort is intentional for consistent downstream iteration order.
• Risk: Very low. Only changes the ORDER of matched pairs in the result, not WHICH pairs are matched. Downstream code iterates matched pairs for patch computation, and order should not affect correctness.
• Verify round-trip: N/A -- order of matched pairs doesn't affect round-trip identity.
• Verify syncback parity: N/A
• Tests required:
  • No new tests needed. Existing tests verify correct pairings, not ordering.

Fix 5: Cache VCache at Session Level in Lua Matching

• Status: Approved
• Finding: Performance -- redundant cacheVirtual calls
• Files: plugin/src/Reconciler/matching.lua
• Problem: cacheVirtual is called during ambiguous group scoring (line 492) AND again inside computePairCost (line 362). The same virtual instance's properties are decoded multiple times per session.
• Solution:
  • Add a vCacheStore: { [string]: VCache } field to MatchingSession.
  • In cacheVirtual, check session.vCacheStore[virtualId] first. If present, return cached. Otherwise, compute and store.
  • Update computePairCost to use the session-level cache instead of re-calling cacheVirtual.
  • The classKeys parameter varies by group, but for a given virtualId the classKeys is always the same (determined by ClassName). So caching by virtualId is safe.
• Risk: Very low. Pure performance optimization. The VCache content is deterministic for a given virtualId.
• Verify round-trip: N/A
• Verify syncback parity: N/A
• Tests required:
  • No new tests needed. Existing matching.spec.lua tests verify correctness. Could add a timing test but not required.

Fix 6: Add Parity Tests Between All 3 Implementations

• Status: Approved
• Finding: Steps 5-6: Missing parity and round-trip tests
• Files: tests/tests/matching_fixtures.rs (extend), plugin/src/Reconciler/matching.spec.lua (extend)
• Problem: No tests verify that the 3 implementations produce identical pairings for the same input. Given Fixes 1-3 close the scoring gaps, parity tests are needed to prevent future divergence.
• Solution:
  • Rust integration test (tests/tests/matching_fixtures.rs): Add a test parity_forward_vs_syncback that:
    1. Creates a WeakDom with an ambiguous group (e.g., 4 Parts named "Line" with different Transparency values, in reversed order)
    2. Runs syncback matching on (new_children, old_children)
    3. Builds a RojoTree from new_children snapshots, runs forward sync matching on (old_children_as_snapshots, tree_children)
    4. Verifies both produce the same pairings (by comparing matched Name+Property values)
  • Lua spec test (matching.spec.lua): Add a test parity_with_rust_fixture that:
    1. Uses the same input data as the Rust test (4 Parts with specific Transparency values)
    2. Runs Lua matching
    3. Verifies the same pairings as the Rust tests (by property comparison)
  • Round-trip matching test (tests/tests/matching_fixtures.rs): Add round_trip_matching_identity:
    1. Uses the UFOWave fixture (12 Textures with default-omitted Face)
    2. Runs syncback to produce filesystem
    3. Runs forward sync on the filesystem output
    4. Verifies all 12 Textures are matched to their correct counterparts (by Face value)
• Risk: None. Tests only.
• Verify round-trip: This IS the round-trip test.
• Verify syncback parity: N/A (these are matching-level tests, not filesystem-level)
• Tests required: (this fix IS the test)

Skipped Fixes

None. All identified fixes were approved.

Accepted Limitations

Greedy Matching Not Globally Optimal

• Limitation: The greedy assignment algorithm is not globally optimal. Edge cases exist where a locally-best pick forces a globally-worse assignment.
• Scenario: Large ambiguous groups (10+ instances) with specific cost patterns where the Hungarian algorithm would produce different (better) pairings.
• User decision: Accepted for all releases (documented in atlas-dedup.mdc).
• Mitigation: Ambiguous groups are typically small (2-10 instances). The depth limit (MAX_SCORING_DEPTH=3) bounds worst-case performance. Monitoring via test coverage for large groups.

Deferred Refactors

• Shared matching core: The 3 implementations share the same algorithm but differ in input types (InstanceSnapshot vs WeakDom vs Studio Instance). A generic/trait-based approach could consolidate the duplicated logic (~2700 total lines), but the type differences are fundamental to each context. Estimated scope: 10+ files, significant API redesign. Deferred.
• Duplicated grouping pattern: All 3 implementations repeat the same HashMap-based (Name, ClassName) -> Vec<index> grouping. Could be factored into a shared helper, but types differ. Minor DRY violation. Deferred.

Test Plan

Rust Unit Tests (#[cfg(test)] blocks)

• ref_scoring_forward_sync -- 2 Models with different PrimaryPart targets, verify correct pairing (Fix 1)
• ref_scoring_syncback -- same scenario for syncback matching (Fix 1)
• tags_granular_scoring -- 2 Parts with different tag sets, verify scoring (Fix 2)
• attributes_granular_scoring -- 2 Parts with different attribute maps, verify scoring (Fix 2)
• Update existing variant_eq float tests for new epsilon behavior (Fix 3)
• fuzzy_eq_matches_lua -- verify same boundary values as trueEquals.spec.lua (Fix 3)

Rust Integration Tests (tests/tests/)

• parity_forward_vs_syncback -- same ambiguous group through both Rust matchers, verify identical pairings (Fix 6)
• round_trip_matching_identity -- UFOWave fixture through syncback then forward sync, verify correct pairings (Fix 6)

Lua Spec Tests (.spec.lua files)

• parity_with_rust_fixture -- same input as Rust parity test, verify identical pairings (Fix 6)

Snapshot Tests (insta crate)

• None needed for matching fixes.

Failing Tests for Known Limitations

• None. The greedy limitation is accepted and no test is needed to demonstrate it.

Test Rules

The implementer MUST follow these rules when writing tests for this plan:

For bugs found and fixed

Every bug fix MUST have a test that:

1. Would have FAILED before the fix
2. PASSES after the fix
3. Prevents the bug from regressing

For missing coverage identified

For each "missing test coverage" gap:

1. Write the test
2. Verify it passes against the current implementation
3. If it fails, investigate -- it may have found another bug

For known limitations (Accepted Limitations section)

No known-limitation tests needed for this plan.

Scope: test every affected layer

For each finding, determine which test layers are affected and write tests in ALL of them:

• Does the bug manifest at the plugin level? -> Lua spec test
• Does it manifest at the server level? -> Rust unit test or integration test
• Does it affect the round-trip? -> Integration test with /api/write + filesystem verification
• Does it affect forward-sync? -> Serve test with recv_socket_packet
• Could it regress from a different code path? -> Test that code path too

Final Step: Run CI

After ALL fixes and tests are implemented, run the /ci command (see .cursor/commands/ci.md) to execute the full CI pipeline. Every fix must pass CI before the plan is considered complete. Do not skip this step.