rojo/.cursor/plans/audit_slugify_pass_3_b11f65a1.plan.md

name	overview	todos	isProject
Audit slugify pass 3	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.	id content status c1-model-rename CRITICAL: Fix model JSON rename in change_processor.rs -- add .model to suffix list, update name inside model file instead of adjacent meta completed id content status cc1-model-update-name 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 completed id content status cc2-dir-removal-meta Fix remove_instance_at_path directory case to clean up grandparent-level DirName.meta.json5 completed id content status test-roundtrip-slugified Add roundtrip test: instances with forbidden chars through syncback -> rebuild -> compare completed id content status test-model-rename Add two-way sync test: rename a .model.json5 instance, verify compound extension preserved and name updated completed id content status test-idempotency Add syncback idempotency integration test: syncback twice = zero changes completed id content status test-3-collision Add 3+ instance collision test (X/Y, X:Y, X|Y all slug to X_Y) completed id content status test-reserved-names Add Windows reserved name integration test (CON, PRN round-trip) completed id content status test-dir-rename Add two-way sync test: directory/init-file rename with forbidden chars completed id content status test-case-collision Add case-insensitive collision integration test (Foo vs foo) completed id content status test-meta-empty-deletion Strengthen rename_slugified_to_clean test to verify meta file is deleted (not just name removed) when it becomes empty completed	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 lines 786-868

The rename handler's suffix list:

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 lines 1091-1098

When updating an existing .model.json5 via two-way sync (plugin sends updated properties), the code rewrites the entire model file:

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 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