hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-25 04:54:07 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Add audit plans for ambiguous paths fixes and follow-up actions

Browse source 
- Introduced two new plan files: one detailing fixes for issues identified during the ambiguous-paths branch audit, including recursive matching, ref path formatting, and dedup cleanup improvements.
- The second plan outlines follow-up actions required for production audit, addressing critical round-trip issues, metadata consistency, and necessary regression tests.
- Documented todos for each plan to ensure clarity on implementation steps and testing requirements.

These additions enhance the auditing process and provide a structured approach to resolving identified issues, improving overall project reliability and maintainability.
This commit is contained in:
Joe 2026-02-17 05:41:53 -06:00
parent e207a398b1
commit f2cdb059c5
2 changed files with 491 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

181
.cursor/plans/audit_ambiguous_paths_followup_00831072.plan.md Normal file
View file

@ -0,0 +1,181 @@
---
name: audit ambiguous paths followup
overview: Production audit of the ambiguous-paths branch against origin/master. Documents critical round-trip issues, approved fixes, accepted limitations, and required regression tests for implementation in a follow-up Agent-mode session.
todos:
- id: fix-metadata-drift
content: Update dedup cleanup rename flow to keep tree metadata/path indexes in sync
status: pending
isProject: false
---
# Ambiguous Paths Audit Fix Plan
> 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. Small helper refactors are in scope; major pipeline redesign is deferred.
## Context
Audited branch `ambiguous-paths` against `origin/master` using commit range `origin/master..HEAD`.
Primary findings:
- Critical path drift in dedup cleanup rename flow (`[src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)`).
- Critical same-batch Ref target path synthesis mismatch for added instances (`[src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs)`).
- Critical ref-path post-processing strategy relies on global text replacement (`[src/syncback/mod.rs](d:/UserGenerated/rojo/src/syncback/mod.rs)`, `[src/syncback/fs_snapshot.rs](d:/UserGenerated/rojo/src/syncback/fs_snapshot.rs)`).
- Known limitation accepted for this release: duplicate-named children under ProjectNode currently skipped in `[src/snapshot_middleware/project.rs](d:/UserGenerated/rojo/src/snapshot_middleware/project.rs)`.
- Known limitation accepted for this release: greedy matcher is not globally optimal (`[src/snapshot/matching.rs](d:/UserGenerated/rojo/src/snapshot/matching.rs)`, `[src/syncback/matching.rs](d:/UserGenerated/rojo/src/syncback/matching.rs)`, `[plugin/src/Reconciler/matching.lua](d:/UserGenerated/rojo/plugin/src/Reconciler/matching.lua)`).
## Fixes
Each fix below was approved by the user during the audit quiz. Implement in order.
### Fix 1: Keep tree metadata consistent after dedup cleanup rename
- **Status:** Approved
- **Finding:** Step 2d / Step 8 (rename/delete interactions + VFS suppression)
- **Files:** `[src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)`
- **Problem:** In dedup cleanup (`~1073-1203`), the code renames files (`fs::rename`) and suppresses both old/new paths, but does not update the renamed sibling's `InstigatingSource::Path` or metadata path indexes. Subsequent writes can target stale paths.
- **Solution:** After successful `DedupCleanupAction::RemoveSuffix` / `PromoteLowest`, locate the sibling instance being renamed, update its metadata source path and relevant path indexes in-memory, and keep suppression counters consistent with the updated path.
- **Risk:** Medium; touches tree mutation bookkeeping and rename lifecycle.
- **Verify round-trip:** Create duplicate siblings (`Foo`, `Foo~1`), delete `Foo`, ensure `Foo~1 -> Foo` rename occurs, then issue a property update to the survivor in same session and verify disk write targets new path and survives rebuild.
- **Verify syncback parity:** Compare output of two-way deletion flow to CLI syncback on equivalent tree state; both must produce the same remaining filename/meta layout.
- **Tests required:**
- Rust integration test in `[tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs)` covering delete-triggered dedup cleanup followed by update.
- Optional focused unit test around cleanup metadata path update helper in `[src/change_processor.rs](d:/UserGenerated/rojo/src/change_processor.rs)`.
### Fix 2: Build correct same-batch added Ref paths
- **Status:** Approved
- **Finding:** Step 2a / Step 10 (plugin->server wire format and write processing)
- **Files:** `[src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs)`
- **Problem:** `added_paths` map (`~622-649`) uses only slugified instance name; it omits middleware extension and dedup suffix. `syncback_updated_properties` then writes incorrect `Rojo_Ref_`* when target is newly added in same request (`~2694-2706`).
- **Solution:** Derive same-batch added target path using the same filesystem naming pipeline as syncback write (class/middleware extension + dedup suffix + directory-vs-file representation). Reuse/shared helper preferred to avoid divergence.
- **Risk:** Medium; affects same-request Ref persistence edge path.
- **Verify round-trip:** Send `/api/write` with add(target script/model) + update(ref property pointing to target) in one batch; verify persisted path includes correct segment (`.server.luau`, `.model.json5`, or dedup suffix) and resolves after rebuild.
- **Verify syncback parity:** Run equivalent scenario via CLI syncback fixture and compare written `Rojo_Ref_`* attribute string.
- **Tests required:**
- Rust integration test in `[tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs)` for same-batch add+ref on file-backed and directory-backed targets.
- Edge test for dedup suffix target (e.g., `Foo~1`).
### Fix 3: Replace global ref-path text rewriting with attribute-scoped final paths
- **Status:** Approved
- **Finding:** Step 2a / Step 9 / Step 11h (round-trip identity, parity, determinism)
- **Files:** `[src/syncback/mod.rs](d:/UserGenerated/rojo/src/syncback/mod.rs)`, `[src/syncback/ref_properties.rs](d:/UserGenerated/rojo/src/syncback/ref_properties.rs)`, `[src/syncback/fs_snapshot.rs](d:/UserGenerated/rojo/src/syncback/fs_snapshot.rs)`, `[src/syncback/snapshot.rs](d:/UserGenerated/rojo/src/syncback/snapshot.rs)`
- **Problem:** Syncback currently calls `collect_referents(..., None)` (`~177`) then performs broad post-hoc string replacement (`~614-637`) via `fix_ref_paths` (`~456-492`) across entire meta/model file text. This is not attribute-scoped and can alter unrelated strings.
- **Solution:** Feed definitive final paths into ref linking directly (e.g., pass populated `final_paths` to `collect_referents`) so `Rojo_Ref_`* attributes are written correctly first time. Remove or strictly confine generic text replacement path.
- **Risk:** Medium-high; touches syncback ref pipeline and ordering.
- **Verify round-trip:** Scenario with duplicate-named ref target and non-ref string property containing same substring as tentative path; ensure only `Rojo_Ref_`* changes and non-ref string is untouched.
- **Verify syncback parity:** Compare CLI syncback and two-way outputs for refs targeting dedup-suffixed instances; paths must be byte-identical.
- **Tests required:**
- Rust integration test in `[tests/tests/syncback_roundtrip.rs](d:/UserGenerated/rojo/tests/tests/syncback_roundtrip.rs)` validating non-ref string preservation while refs are rewritten correctly.
- Rust integration test in `[tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs)` for dedup-aware ref persistence without text rewrite side effects.
### Fix 4: Add regression coverage for approved fixes
- **Status:** Approved
- **Finding:** Step 13 (missing coverage)
- **Files:** `[tests/tests/two_way_sync.rs](d:/UserGenerated/rojo/tests/tests/two_way_sync.rs)`, `[tests/tests/syncback_roundtrip.rs](d:/UserGenerated/rojo/tests/tests/syncback_roundtrip.rs)`, optionally helper fixtures under `[rojo-test/syncback-tests](d:/UserGenerated/rojo/rojo-test/syncback-tests)`.
- **Problem:** Current tests do not fully guard the three approved bug classes.
- **Solution:** Add targeted regression tests tied to Fixes 1-3 and ensure they fail pre-fix and pass post-fix.
- **Risk:** Low.
- **Verify round-trip:** Covered by added integration scenarios.
- **Verify syncback parity:** Add assertions comparing resulting file paths/attribute values against expected CLI-format output.
- **Tests required:**
- All tests listed under Fixes 1-3.
- One deterministic re-run assertion (second run no changes) for affected scenarios.
## Skipped Fixes
### Skipped: ProjectNode duplicate-child handling rewrite
- **Finding:** Step 2d / Step 11k
- **Problem:** Duplicate-named children under ProjectNode are currently skipped with warnings (`[src/snapshot_middleware/project.rs](d:/UserGenerated/rojo/src/snapshot_middleware/project.rs)` around `~587-683`).
- **Reason skipped:** User accepted as release limitation for now.
### Skipped: Globally optimal matcher replacement
- **Finding:** Step 11h (determinism/assignment quality)
- **Problem:** Greedy assignment may produce suboptimal pairing in rare ambiguous groups.
- **Reason skipped:** User accepted greedy behavior for this release.
## Accepted Limitations
- **Limitation:** ProjectNode duplicate-named children are skipped during syncback.
- **Scenario:** Under project-backed nodes, duplicate names are detected and omitted from child maps; warning logged.
- **User decision:** Accepted for this release.
- **Mitigation:** Keep explicit warning logs; add failing acceptance test (below) to track until fixed.
- **Limitation:** Greedy matching is not globally optimal.
- **Scenario:** Ambiguous duplicate groups can choose a local minimum causing rename churn.
- **User decision:** Accepted for this release.
- **Mitigation:** Add stress/failing test for known counterexample and keep deterministic tie behavior.
## Deferred Refactors
- Consolidate filesystem-segment derivation into one shared helper used by `[src/rojo_ref.rs](d:/UserGenerated/rojo/src/rojo_ref.rs)`, `[src/snapshot/tree.rs](d:/UserGenerated/rojo/src/snapshot/tree.rs)`, and `[src/web/api.rs](d:/UserGenerated/rojo/src/web/api.rs)` to eliminate duplicate naming logic.
- Revisit optimal assignment algorithm for large ambiguous groups (major rewrite; out of current scope).
## Test Plan
### Rust Unit Tests (`#[cfg(test)]`)
- Dedup cleanup metadata update helper behavior after rename (Fix 1).
- Added-path segment generation helper returns correct extension/suffix across middleware types (Fix 2).
### Rust Integration Tests (`tests/tests/`)
- Delete base dedup name then update promoted sibling in same serve session; verify writes hit new path and persist after rebuild (Fix 1).
- Same `/api/write` request: add target + set Ref to target, verify persisted path includes expected extension/suffix (Fix 2).
- Ref rewrite safety: non-ref string fields are unchanged while `Rojo_Ref_`* gets final dedup path (Fix 3).
### Lua Spec Tests (`.spec.lua`)
- No mandatory Lua-side changes for approved fixes; optional guard test for same-batch add+ref behavior if plugin-side helper touched.
### Snapshot Tests (`insta`)
- Add or update syncback snapshot fixture showing final dedup-aware `Rojo_Ref_`* output without incidental string changes (Fix 3).
### Failing Tests for Known Limitations
- ProjectNode duplicate-named child round-trip identity test asserting correct behavior (expected to fail until limitation removed).
- Ambiguous-group matching counterexample test asserting globally optimal pairing (expected to fail with greedy algorithm).
## Test Rules
### For bugs found and fixed
- Each approved bug fix must have a regression test that fails pre-fix and passes post-fix.
### For missing coverage identified
- Add coverage in affected layer(s); investigate any unexpected failure as a potential additional bug.
### For known limitations (Accepted Limitations section)
- Write correctness-asserting tests even if they fail now.
- Leave them failing (do not ignore), to serve as future acceptance criteria.
### Scope
- Cover both persistence correctness (filesystem outputs) and in-memory tree consistency where applicable.
## Final Step: Run CI
After implementing all approved fixes and tests, run `/ci` and require a clean pass before considering this plan complete.