mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Add audit plan for slugify branch pass 3 and implement critical fixes
- Introduced a comprehensive audit plan for the slugify branch, detailing findings from a holistic review of 110 files, including 1 critical bug and 2 correctness issues. - Completed todos addressing model JSON renaming, syncback updates, and test enhancements for roundtrip fidelity. - Added tests for reserved names and ensured correct handling of slugified instances, preserving compound extensions in model files. - Refactored change processing logic to correctly manage the `name` field in model files, enhancing metadata handling and ensuring compliance with round-trip identity standards.
This commit is contained in:
Joe
parent
2a917bf8e1
commit
bb266789c0
14 changed files with 843 additions and 23 deletions
162
.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md
Normal file
162
.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md
Normal file
|
|
@ -0,0 +1,162 @@
|
|||
---
|
||||
name: Audit slugify pass 3
|
||||
overview: Holistic audit of the entire `slugify` branch diff against master (110 files, +6315/-1188 lines, 21 commits). Found 1 critical bug, 2 correctness issues, and 8 test gaps. All prior plan fixes verified in code. Legacy cleanup confirmed complete.
|
||||
todos:
|
||||
- id: c1-model-rename
|
||||
content: "CRITICAL: Fix model JSON rename in change_processor.rs -- add .model to suffix list, update name inside model file instead of adjacent meta"
|
||||
status: completed
|
||||
- id: cc1-model-update-name
|
||||
content: Fix syncback_update_existing_instance (api.rs:1093) dropping name field when rewriting .model.json5 files, and write_init_meta_json (api.rs:1075) dropping name field for directories
|
||||
status: completed
|
||||
- id: cc2-dir-removal-meta
|
||||
content: Fix remove_instance_at_path directory case to clean up grandparent-level DirName.meta.json5
|
||||
status: completed
|
||||
- id: test-roundtrip-slugified
|
||||
content: "Add roundtrip test: instances with forbidden chars through syncback -> rebuild -> compare"
|
||||
status: completed
|
||||
- id: test-model-rename
|
||||
content: "Add two-way sync test: rename a .model.json5 instance, verify compound extension preserved and name updated"
|
||||
status: completed
|
||||
- id: test-idempotency
|
||||
content: "Add syncback idempotency integration test: syncback twice = zero changes"
|
||||
status: completed
|
||||
- id: test-3-collision
|
||||
content: Add 3+ instance collision test (X/Y, X:Y, X|Y all slug to X_Y)
|
||||
status: completed
|
||||
- id: test-reserved-names
|
||||
content: Add Windows reserved name integration test (CON, PRN round-trip)
|
||||
status: completed
|
||||
- id: test-dir-rename
|
||||
content: "Add two-way sync test: directory/init-file rename with forbidden chars"
|
||||
status: completed
|
||||
- id: test-case-collision
|
||||
content: Add case-insensitive collision integration test (Foo vs foo)
|
||||
status: completed
|
||||
- id: test-meta-empty-deletion
|
||||
content: Strengthen rename_slugified_to_clean test to verify meta file is deleted (not just name removed) when it becomes empty
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Audit: slugify Branch -- Pass 3 (Holistic)
|
||||
|
||||
## Scope
|
||||
|
||||
Full diff against `master`: 110 files, +6,315 / -1,188 lines across 21 commits. Every changed file reviewed. All 4 prior plan files cross-referenced against codebase.
|
||||
|
||||
## Quality Standard
|
||||
|
||||
> **Round-trip identity**: 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**. Any deviation is a bug.
|
||||
|
||||
---
|
||||
|
||||
## Prior Plan Verification
|
||||
|
||||
All claims from prior plans verified in code -- no discrepancies:
|
||||
|
||||
- **fix_audit_findings**: C1 (meta gating), C2 (removal meta path), C3 (deny_unknown_fields), CC1 (determinism), CC2 (init removal), DR1 (shared meta helper), DR2 (meta path helper) -- all confirmed in code.
|
||||
- **fix_audit_pass_2**: CC2 refactor (`remove_instance_at_path`), preseed fix, rename dedup, dir sort, csv instigating_source, over-suppression, comment fix -- all confirmed.
|
||||
- **fix_stem-level_dedup**: `return-dedup-key`, `fix-dir`, `fix-project`, `fix-api` -- all confirmed. Note: `fix-tests` is still marked `in_progress` in the plan but tests do exist; the plan status is stale.
|
||||
|
||||
---
|
||||
|
||||
## Areas Confirmed CLEAN
|
||||
|
||||
Thoroughly re-audited via the full diff and found correct:
|
||||
|
||||
- **Forward sync** (all middleware): `decode_path_name` fully removed. All middleware correctly resolves names from metadata `name` field or filename stem. `specified_name` set on `InstanceMetadata` in every middleware. `is_empty()` updated for `name`. `instigating_source` preserved in `csv.rs`.
|
||||
- **Reverse sync / syncback**: `slugify_name()`, `deduplicate_name()`, `name_for_inst()` all correct. Pre-seeding uses `relevant_paths` + `strip_middleware_extension()`. Child ordering sorted alphabetically in both `dir.rs` and `project.rs`. `dedup_key.to_lowercase()` applied at ALL insertion sites.
|
||||
- **Two-way sync adds**: `syncback_added_instance` groups adds by parent, seeds `sibling_slugs` from tree children via `instigating_source` paths, uses `strip_middleware_extension`. Meta files written when slug differs. Meta path matches script file path via `adjacent_meta_path()`.
|
||||
- **Two-way sync removals**: `remove_instance_at_path` handles directories, init files (parent dir removal + grandparent meta), and regular files (adjacent meta). Old dead `syncback_removed_instance` deleted.
|
||||
- **Meta lifecycle**: C1 fix in place -- meta updates OUTSIDE `if new_path != *path` for both init and regular paths. Shared module (`syncback::meta`) used by `change_processor.rs`. `suppress_path_remove` exists. `RemoveNameOutcome` handled correctly with `(1, 0)` counts for `FileDeleted`.
|
||||
- **Legacy cleanup**: Zero references to old encoding system in source, tests, plugin, schema, or snapshots. `src/path_encoding.rs` deleted. `pub mod path_encoding` removed from `lib.rs`. `decode_windows_invalid_chars` removed from `InstanceContext` and `serve_session.rs`.
|
||||
- **Dedup key consistency**: All insertion sites use `.to_lowercase()`. Cross-format collision handled.
|
||||
- **SyncbackRules**: No `deny_unknown_fields`.
|
||||
- **JSON schema**: `encodeWindowsInvalidChars` removed from `vscode-rojo/schemas/`.
|
||||
- **Snapshot files**: All `.snap` files updated. No stale `decode_windows_invalid_chars` or `%ENCODED%` patterns.
|
||||
- `**json_model_legacy_name` snapshot change**: Instance name changed from `Expected Name` to `Overridden Name` -- correct, this is the intended behavior change (the `Name`/`name` field is now respected instead of ignored).
|
||||
- **Test fixture migration**: All `%ENCODED%` filenames in `syncback_encoded_names/` correctly replaced with slugified names + meta files.
|
||||
- **Existing test updates**: All ~12 two-way sync tests with encoded patterns correctly updated to slugified equivalents.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL: Model JSON rename loses `.model` compound extension
|
||||
|
||||
**File**: [src/change_processor.rs](src/change_processor.rs) lines 786-868
|
||||
|
||||
The rename handler's suffix list:
|
||||
|
||||
```786:789:src/change_processor.rs
|
||||
let known_suffixes = [
|
||||
".server", ".client", ".plugin", ".local",
|
||||
".legacy",
|
||||
];
|
||||
```
|
||||
|
||||
Does NOT include `.model`. For `What_Model.model.json5` renamed to `Why?Model`:
|
||||
|
||||
- `stem` = `"What_Model.model"`, `extension` = `"json5"`, `script_suffix` = `""` (no match)
|
||||
- `old_base` = `"What_Model.model"` (includes `.model`)
|
||||
- New filename = `"Why_Model.json5"` -- `.model` LOST
|
||||
|
||||
**Impact**: File type changes from `.model.json5` (JSON instance) to `.json5` (data module). Instance class, properties, hierarchy silently lost. The meta update then writes to `Why_Model.meta.json5` (wrong location for model files). Direct round-trip violation.
|
||||
|
||||
**Fix**: Add `.model` to the suffix list. Then detect `.model.json5`/`.model.json` files and update the `name` field INSIDE the model file (like `syncback::meta::upsert_meta_name` but targeting the JSON root's `name` key) instead of writing adjacent `.meta.json5`.
|
||||
|
||||
---
|
||||
|
||||
## CORRECTNESS: `syncback_update_existing_instance` drops `name` field
|
||||
|
||||
**File**: [src/web/api.rs](src/web/api.rs) lines 1091-1098
|
||||
|
||||
When updating an existing `.model.json5` via two-way sync (plugin sends updated properties), the code rewrites the entire model file:
|
||||
|
||||
```1093:1093:src/web/api.rs
|
||||
let content = self.serialize_instance_to_model_json(added, None)?;
|
||||
```
|
||||
|
||||
`None` for `instance_name` means the `name` field is NOT written. If the model file previously had `"name": "Hey/Bro"`, the name is silently lost on rewrite. Same issue exists at line 1075 for directory updates via `write_init_meta_json(existing_path, added, None)`.
|
||||
|
||||
**Fix**: Compute `meta_name_field` by comparing the slug of the existing filesystem name against `added.name`. If they differ, pass `Some(&added.name)`. This matches the logic in `syncback_instance_to_path_with_stats`.
|
||||
|
||||
---
|
||||
|
||||
## CORRECTNESS: `remove_instance_at_path` directory case skips grandparent meta
|
||||
|
||||
**File**: [src/web/api.rs](src/web/api.rs) lines 1389-1396
|
||||
|
||||
When `path.is_dir()` (plain Folder), `remove_dir_all` is called but no adjacent `DirName.meta.json5` at the parent level is cleaned up. The init-file case (lines 1423-1438) correctly handles this.
|
||||
|
||||
**Severity**: LOW. Grandparent-level `DirName.meta.json5` only exists from manual creation or the change_processor rename path (which renames it). Standard syncback uses `init.meta.json5` inside the directory.
|
||||
|
||||
**Fix**: After `remove_dir_all`, add grandparent meta cleanup matching the init-file path.
|
||||
|
||||
---
|
||||
|
||||
## TEST COVERAGE GAPS
|
||||
|
||||
Prioritized by risk:
|
||||
|
||||
1. **Roundtrip test with slugified names** -- `syncback_roundtrip.rs` exists but no fixtures exercise slugified names. None of `meta_name_override`, `model_json_name_override`, `dedup_suffix_with_meta` are in the roundtrip test list.
|
||||
2. **Model file rename two-way sync** -- No test for renaming a `.model.json5` instance. Would catch the critical bug above.
|
||||
3. **Syncback idempotency integration** -- Run syncback twice, assert zero changes on second run. Unit tests exist but no full-pipeline test.
|
||||
4. **3+ instance collision** -- Only 2-instance collision tested (`add_two_colliding_instances_deduplicates`). Need `X/Y`, `X:Y`, `X|Y` -> `X_Y`, `X_Y~1`, `X_Y~2`.
|
||||
5. **Windows reserved name integration** -- Unit tests for `CON`/`PRN` slugification exist. No build or syncback integration test.
|
||||
6. **Directory rename with forbidden chars** -- Two-way sync tests cover file renames but not init-file/directory renames.
|
||||
7. **Case-insensitive collision** -- Code handles it (`.to_lowercase()` everywhere). No integration test.
|
||||
8. **Meta file empty deletion** -- `rename_slugified_to_clean` test checks meta doesn't have `name` field but doesn't verify the meta file itself is deleted when it becomes empty (the `RemoveNameOutcome::FileDeleted` path).
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
|
||||
| Category | Count | Details |
|
||||
| ------------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| Critical bugs | 1 | Model JSON rename loses `.model` extension |
|
||||
| Correctness | 2 | `syncback_update_existing_instance` drops `name` field; directory removal skips grandparent meta |
|
||||
| Missing tests | 8 | Roundtrip, model rename, idempotency, 3+ collision, reserved names, dir rename, case collision, meta empty deletion |
|
||||
| Prior plans | OK | All claims verified in code, no discrepancies |
|
||||
| Clean areas | 10 | Forward sync, reverse sync, two-way adds/removals, meta lifecycle, legacy cleanup, dedup consistency, schema, snapshots, test fixtures, test updates |
|
||||
|
||||
|
||||
Loading…
Add table
Add a link
Reference in a new issue