hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 14:01:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Add audit plan for Ambiguous Path Handling feature

Browse source 
- Introduced a comprehensive audit plan detailing critical issues, code quality items, and test coverage gaps identified in the Ambiguous Path Handling feature.
- Documented specific todos addressing various correctness and performance issues, including case sensitivity, path formatting, and dead code removal.
- Established standards for round-trip identity and CLI syncback parity to ensure consistent behavior across the system.

These changes enhance the clarity and accountability of the auditing process, paving the way for improved feature reliability and maintainability.
This commit is contained in:
Joe 2026-02-17 03:43:27 -06:00
parent e313f61515
commit 4cb45cd607
6 changed files with 648 additions and 154 deletions
Download patch file Download diff file Expand all files Collapse all files

212
.cursor/plans/audit_ambiguous_path_handling_30e99d9a.plan.md Normal file
View file

@ -0,0 +1,212 @@
---
name: Audit Ambiguous Path Handling
overview: Production audit of the Ambiguous Path Handling feature (matching algorithm, dedup system, ref path system). Identifies 7 critical/correctness issues, 3 code quality items, and extensive missing test coverage."
todos:
- id: c1-pass3-double-match
content: "Fix: Forward sync pass3 can re-match pass1-matched items (snap_available not consumed)"
status: completed
- id: c2-case-sensitivity
content: "Fix: get_instance_by_path inconsistent case sensitivity between primary and fallback"
status: completed
- id: c3-projectnode-path
content: "Fix: ref_target_path_from_tree uses project filename instead of instance name for ProjectNode"
status: completed
- id: c5-return-value
content: "Verify: encodeInstance return value change doesn't break callers"
status: completed
- id: c6-dead-code
content: "Fix: Remove dead code in ref_properties.rs (compute_refs_with_duplicate_siblings, is_path_unique_with_cache)"
status: completed
- id: c7-ref-path-format
content: "Critical: ref_properties.rs writes instance-name paths but tree.rs expects filesystem-name paths"
status: completed
- id: q2-hashset-perf
content: "Fix: Use HashSet instead of Vec for matched tracking in syncback matching"
status: completed
- id: c8-matching-lua-require-crash
content: "Critical: matching.lua has wrong require path for Log module - crashes on load, breaks hydration entirely"
status: completed
- id: q3-unused-import
content: "Fix: Remove unused Log import in Lua matching module (after fixing c8, Log is still unused)"
status: completed
- id: t1-syncback-integration
content: "Test: Add syncback integration tests for duplicate-named instances"
status: completed
- id: t3-new-fn-tests
content: "Test: Add tests for ref_target_path_from_tree and filesystem_name_for"
status: completed
isProject: false
---
# Audit: Ambiguous Path Handling Feature
> This plan was generated by `/audit` from a Plan-mode session.
> Refer to `.cursor/rules/atlas.mdc` for full project standards.
## Standards
### Round-Trip Identity (from `atlas.mdc`)
Syncback writes a directory tree. Building an rbxl from that tree and forward-syncing it back must produce a **bit-identical instance tree**. Any deviation is a bug.
### CLI Syncback Parity (from `atlas.mdc`)
Plugin-based sync must produce **exactly the same filesystem output** as `atlas syncback`. Any divergence is a bug.
### Code Quality (from `atlas.mdc`)
DRY code, no duplicated slugify/dedup/meta patterns across files.
## Context
Auditing all changes on `ambiguous-paths` branch (4 commits, 24 files, +3560/-825 lines) against `origin/master`. The feature replaces the duplicate-skip blacklist with a 3-pass matching algorithm, filesystem-collision-based dedup, and filesystem-name-based ref paths.
---
## Critical Issues
### C1. Forward sync matching does not filter already-matched items in Pass 3
**File:** [src/snapshot/matching.rs](src/snapshot/matching.rs), `pass3_similarity()` line 199-262
**Problem:** `pass3_similarity` builds its own `snap_by_name` / `tree_by_name` maps from ALL items in `snap_available` / `tree_children`, including items already matched in Pass 1 (which are marked only via `snap_matched[si]=true` / `tree_available[ti]=false` in the caller). However, `snap_by_name` iterates `snap_available` checking `if let Some(snap) = snap_opt` -- but snap_available entries are never set to `None` after Pass 1 matching. This means Pass 3 **re-groups items already matched in Pass 1** into its name maps. The `snap_matched` guard inside the pair-building loop catches this for snap items, but the `tree_by_name` check is `if !tree_available[i]` which IS correctly applied. The snap side relies on `snap_matched[si]` which is initialized to `vec![false; ...]` at the top of pass3 -- it does NOT inherit the pass1 match state.
**Impact:** Pass 3 will attempt to re-match snapshots already matched in Pass 1, potentially creating duplicate matched pairs in the output. The `build_result()` function does `.take()` on snap entries, so the second `.take()` would return `None` and the pair would be silently dropped. This means the unmatched counts would be wrong (items appear in neither matched nor unmatched).
**Fix:** Either set `snap_available[si] = None` for pass1-matched items before calling pass3, or initialize pass3's `snap_matched` from the caller's `snap_matched` state. The simplest fix: filter `snap_by_name` to only include indices where `snap_available[i].is_some()` AND the index was not already matched. Since we use a separate `snap_matched` bool vec, we can check: the `snap_available` entries for pass1-matched items were never consumed (they're still `Some`). We need to either mark them `None` or pass the matched state.
### C2. `get_instance_by_path` uses case-insensitive comparison for filesystem names but case-SENSITIVE for fallback
**File:** [src/snapshot/tree.rs](src/snapshot/tree.rs), `get_instance_by_path()` line ~248
**Problem:** The primary lookup uses `fs_name.eq_ignore_ascii_case(segment)` but the fallback name lookup uses `child.name == *segment` (case-sensitive). This inconsistency means:
- A ref path `"workspace/Part"` would match filesystem name `"Workspace"` (case-insensitive) in the primary check
- But if the instance had no `instigating_source`, the fallback would fail because `"Workspace" != "workspace"`
More critically, this creates an asymmetry: `ref_target_path_from_tree()` builds paths from filesystem names preserving their original case, but resolution uses case-insensitive matching. This means two distinct instances `"Foo.luau"` and `"foo.luau"` (which should be dedup'd for cross-platform safety) would both match the same path segment.
**Fix:** Use case-insensitive comparison in BOTH the primary and fallback paths. Or use exact case matching in both (since filesystem names are deterministic and paths should round-trip exactly).
### C3. `ref_target_path_from_tree` falls back to instance name for ProjectNode instances, creating inconsistent paths
**File:** [src/rojo_ref.rs](src/rojo_ref.rs), `ref_target_path_from_tree()` line ~157-173
**Problem:** When an instance has `InstigatingSource::ProjectNode`, the code falls through to the `else` branch (no instigating_source) and uses `inst.name.clone()`. But `InstigatingSource::ProjectNode` DOES have a path -- `source.path()` returns the project file path, whose `file_name()` would be something like `default.project.json5`. This is wrong -- for project nodes, the segment should be the instance name (not the project filename).
The current code handles this correctly by accident: the `if let Some(source) = &meta.instigating_source` check matches `ProjectNode` (it has a source), so it uses `source.path().file_name()` which returns the project filename like `"default.project.json5"`. This produces WRONG ref path segments for project-sourced instances -- a service like "Workspace" would get segment `"default.project.json5"` instead of `"Workspace"`.
**Fix:** Check for `InstigatingSource::ProjectNode` specifically and use the instance name, not the project file path. The `instigating_source.path()` for ProjectNode returns the project file itself, not the instance's filesystem representation.
### C4. Lua matching `_pass1NameAndClass` has index invalidation bug when removing from arrays
**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), lines 168-190
**Problem:** After collecting matched indices, the code sorts them in reverse order and calls `table.remove(remainingVirtual, idx)` / `table.remove(remainingStudio, idx)` for each. However, if Pass 1 matched both a unique-name pair AND a class-narrowed pair from the same name group, the same index could theoretically appear twice in `matchedVirtualIndices`. The deduplication check (`seenV[idx]`) handles this.
But the more fundamental issue: indices collected during the matching loop refer to positions in `remainingVirtual` at the START of the function. As `table.remove` shifts elements, subsequent indices become invalid. The reverse-order sort mitigates this for sequential removals, but if indices from DIFFERENT name groups interleave (e.g., index 3 from name "A" and index 2 from name "B"), removing index 3 first then index 2 works. However, if the same index appears for both a unique match and a class match (shouldn't happen since unique match `continue`s), the dedup prevents double-removal.
**Real bug:** The virtual and studio index lists are independent. `matchedVirtualIndices` contains indices into `remainingVirtual`, `matchedStudioIndices` contains indices into `remainingStudio`. Removing from one array doesn't affect the other. But removing from `remainingVirtual` in reverse order IS correct for that array alone. The issue is that indices 1, 3, 5 removed in order 5, 3, 1 correctly removes the right elements.
Actually, this is correct IF no duplicates. But there's a subtle issue: the class narrowing loop can match items whose indices overlap with the unique-name match. Since the unique-name match does `continue` (skips class narrowing for that name), this shouldn't happen. **Low risk but fragile.**
### C5. `encodeInstance.lua` return value change breaks callers expecting 2 return values
**File:** [plugin/src/ChangeBatcher/encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua), bottom
**Problem:** The old function returned `(result, skippedDuplicateCount)` -- two values. The new code returns just `encodeInstance(instance, parentId, false)` -- one value. Per the Lua silent argument mismatch rule, callers that destructured the second return value now silently get `nil` instead of a count. Need to verify all callers.
**Verification needed:** Search for all callers of `encodeInstance` / `encodeInstanceWithDuplicateTracking` to confirm none destructure the second return value.
### C6. Dead code left behind in `ref_properties.rs`
**File:** [src/syncback/ref_properties.rs](src/syncback/ref_properties.rs), lines 54-131
**Problem:** `compute_refs_with_duplicate_siblings()` and `is_path_unique_with_cache()` are now dead code (clippy confirms: "function is never used"). The `collect_referents()` function was simplified to always use path-based linking, but the old helper functions were not removed. This is dead code that will confuse future maintainers.
### C7. `ref_properties.rs` always uses instance-name paths, not filesystem-name paths
**File:** [src/syncback/ref_properties.rs](src/syncback/ref_properties.rs), `collect_referents()` line ~170
**Problem:** The simplified `collect_referents()` calls `inst_path(dom, *target_ref)` which uses the legacy `ref_target_path()` (instance-name-based with escaping). This means Rojo_Ref_* attributes written during syncback still use the OLD path format (instance names), not the new filesystem-name format. But `get_instance_by_path()` in tree.rs now expects filesystem-name segments. This creates a **format mismatch**: syncback writes instance-name paths, forward sync tries to resolve them as filesystem-name paths.
The fallback in `get_instance_by_path()` (case-sensitive instance name match) partially mitigates this, but it's fragile and won't work for instances whose name differs from their filesystem name (slugified names, dedup'd names).
**Impact:** Critical round-trip violation for any instance with a slugified or dedup'd name that is referenced by a Ref property. The ref path written during syncback won't resolve during forward sync.
**Fix:** `collect_referents()` needs to build filesystem-name paths. This requires access to the filesystem names assigned during syncback (via instigating_source or a Ref-to-filesystem-name map built during the syncback pass).
---
### C8. Lua matching module has wrong `require` path -- crashes on load, breaks hydration
**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), line 16
**Problem:** `local Log = require(script.Parent.Parent.Log)` resolves to `plugin/src/Log` which does not exist. Every other file in `plugin/src/Reconciler/` uses `local Packages = script.Parent.Parent.Parent.Packages` followed by `require(Packages.Log)`. Since Luau's `require()` executes immediately at module load time, this incorrect path causes a runtime error when `matching.lua` loads, which prevents the 3-pass matching algorithm from functioning and breaks hydration entirely (`hydrate.lua` line 15 requires `matching`).
**Impact:** The plugin cannot connect to the server at all. Hydration is the first thing that runs after the initial `/api/read` -- if matching.lua fails to load, the entire sync session crashes.
**Fix:** Replace line 16 with:
```lua
local Packages = script.Parent.Parent.Parent.Packages
local Log = require(Packages.Log)
```
---
## Code Quality Issues
### Q1. Duplicated name→index grouping pattern across 3 matching modules
All three matching implementations (syncback, forward sync, Lua plugin) repeat the same pattern: build `HashMap<name, Vec<index>>` for both sides, iterate names, check counts. This is ~40 lines repeated 6+ times across the codebase. Should be factored into a shared helper (or at minimum documented as intentional duplication per the spec requirement of separate implementations).
### Q2. `matched_new.contains(r)` uses O(n) linear scan in syncback matching
**File:** [src/syncback/matching.rs](src/syncback/matching.rs), lines 196, 262, 358, 400
The `matched_new.contains()` / `matched_old.contains()` checks are O(n) linear scans. For large ambiguous groups (10+ duplicates), this creates O(n^2) behavior within Pass 2 and Pass 3. Should use `HashSet` instead of `Vec` for the matched tracking sets.
### Q3. Unused `Log` import in Lua matching module
**File:** [plugin/src/Reconciler/matching.lua](plugin/src/Reconciler/matching.lua), line 16
`local Log = require(script.Parent.Parent.Log)` is imported but never used. Will trigger selene lint warning.
---
## Missing Test Coverage
### T1. No integration tests for duplicate-named instances through syncback
The feature's core value proposition -- syncing duplicate-named instances -- has zero integration tests. The only tests are unit tests for the matching algorithm and dedup suffix cleanup. Need syncback test fixtures with:
- Two Folders with same name
- Folder + Script same name (no collision)
- Init-style collision (Folder + Script with children)
- Roundtrip test with duplicates
### T2. No tests for forward sync with duplicate-named instances
`compute_children_patches` now uses the matching algorithm, but there are no tests verifying that editing one of two same-named files correctly pairs to the right tree instance.
### T3. No tests for `ref_target_path_from_tree` or `filesystem_name_for`
New functions with zero test coverage. Critical for ref path correctness.
### T4. No tests for dedup key semantics change
The `deduplicate_name_with_ext` function is tested via updated existing tests, but there are no tests specifically for the Folder+Script no-collision scenario or init-style collision scenario.
### T5. No Lua spec tests for the matching module
The Lua matching module has zero tests. The `hydrate.lua` changes are untested.
---
## Deferred Refactors
- **Syncback ref path format migration**: `collect_referents()` needs to build filesystem-name paths instead of instance-name paths. This requires restructuring the syncback flow to make filesystem name assignments available during ref collection. (Blocked on: syncback flow restructure)
- **Matching cache (Phase 2e)**: Deferred optimization, not needed for correctness.
- **change_processor dedup cleanup integration**: The `dedup_suffix.rs` module provides cleanup rules but they aren't integrated into the change processor's deletion handling yet.