rojo/.cursor/plans/fix_audit_findings_ca70f773.plan.md

name	overview	todos	isProject
Fix audit findings	Fix all 3 critical bugs, 2 correctness issues, 2 refactors, 1 cleanup item, and add 7 missing tests identified by the slugify branch audit.	id content status dr2-meta-path-helper Extract `adjacent_meta_path()` and `strip_script_suffix()` helper into syncback/file_names.rs completed id content status dr1-shared-meta-update Extract shared meta name update helpers (upsert/remove) from change_processor.rs into syncback module completed id content status c1-meta-gating-fix Fix meta name update gating: move upsert/remove outside path-change condition in change_processor.rs (both init and regular paths) completed id content status c2-removal-meta-path Fix syncback_removed_instance meta path: use filesystem stem instead of instance name (api.rs:1497) completed id content status cc2-init-removal Fix syncback_removed_instance init-file handling: remove parent directory for directory-format scripts completed id content status c3-deny-unknown Remove deny_unknown_fields from SyncbackRules (syncback/mod.rs:966) completed id content status cc1-determinism Sort new_child_map entries before processing in project.rs syncback to ensure deterministic dedup completed id content status cleanup-comment Fix misleading comment at api.rs:1494 about meta file naming completed id content status test-syncback-forbidden Add syncback integration test: forbidden-char instances produce slugified filenames + meta completed id content status test-twoway-add-forbidden Add two-way sync test: add new instance with forbidden chars completed id content status test-twoway-collision Add two-way sync test: two instances with same slug during live sync completed id content status test-idempotency Add idempotency test: syncback twice = zero changes completed id content status test-tilde-e2e Add tilde dedup suffix end-to-end test through build/syncback completed id content status test-negative-tilde Add negative test: Foo~1.luau without meta = instance name Foo~1 completed id content status test-stress Add large tree stress test: 100+ instances with collision patterns completed	false

Fix All Audit Findings

Standards (from audit)

• Round-trip identity: Syncback writes must rebuild to bit-identical instance trees. Any deviation is a bug.
• Code quality: DRY -- no duplicated slugify/dedup/meta patterns. Shared helpers over copy-paste.
• Legacy %ENCODING%: Intentionally unsupported. No backward compat for old %NAME% patterns.

---

C1. Meta name update skipped when slug doesn't change

Files: src/change_processor.rs lines 735-789 (init) and 835-879 (regular)

The meta update calls (upsert_meta_name_field / remove_meta_name_field) are nested inside if new_dir_path != dir_path (line 735) and if new_path != *path (line 835). When renaming between names that slugify identically (e.g. "Foo/Bar" -> "Foo|Bar", both Foo_Bar), the path doesn't change so meta is never updated.

Fix: After each path-change block, add a separate if that checks the name itself. For both the init-file path and regular-file path:

// AFTER the path-change block (regardless of whether path changed):
// Always update meta name field when the instance name changed
let current_meta = /* compute meta path using current (possibly unchanged) path */;
if slugified_new_name != *new_name {
    self.upsert_meta_name_field(&current_meta, new_name);
} else {
    self.remove_meta_name_field(&current_meta);
}

This means if the path DID change, the meta was already renamed to the new location (inside the path-change block), and now we update its name field. If the path did NOT change, we update the existing meta in-place.

---

C2. syncback_removed_instance uses raw instance name for meta path

File: src/web/api.rs line 1497

let meta_path = parent_dir.join(format!("{}.meta.json5", instance_name));

instance_name is the Roblox name (e.g. "Key:Script") -- contains forbidden chars, doesn't match the actual slugified meta file.

Fix: Derive the meta stem from the script file's filename. Strip the middleware extension suffix to get the base name (same slug used when the file was created):

// Derive meta filename from the script's filesystem name, not the instance name
if let Some(file_name) = instance_path.file_name().and_then(|f| f.to_str()) {
    let stem = instance_path.file_stem().and_then(|s| s.to_str()).unwrap_or("");
    // Strip known script suffixes (.server, .client, .plugin, .local, .legacy)
    let base = strip_script_suffix(stem);
    let meta_path = parent_dir.join(format!("{}.meta.json5", base));
    // ... remove if exists
}

This naturally ties into DR2 (shared helper).

---

C3. deny_unknown_fields breaks old project files

Files: src/syncback/mod.rs line 966

SyncbackRules uses #[serde(deny_unknown_fields)]. On master it had encode_windows_invalid_chars. Users upgrading with that field in their project file get a hard parse error.

Fix: Remove deny_unknown_fields from SyncbackRules. This is the simplest approach -- unknown fields are silently ignored, which is the standard serde behavior and the most user-friendly for forward compatibility.

Leave Project struct's deny_unknown_fields as-is since the removed fields (decodeWindowsInvalidChars) were on InstanceContext / SyncbackRules, not on Project directly.

---

CC1. Non-deterministic child ordering in project.rs

File: src/snapshot_middleware/project.rs -- new_child_map is a HashMap iterated via .drain()

When two siblings collide on the same slug, which gets the base name vs ~1 depends on HashMap iteration order -- non-deterministic.

Fix: Collect new_child_map entries into a Vec, sort by name (alphabetical), then process in sorted order. This matches dir.rs behavior where DOM order provides stability:

let mut remaining_children: Vec<_> = new_child_map.drain().collect();
remaining_children.sort_by(|(name_a, _), (name_b, _)| name_a.cmp(name_b));
for (name, new_child) in remaining_children { ... }

---

CC2. syncback_removed_instance doesn't clean up init-file directories

File: src/web/api.rs lines 1487-1513

For directory-format scripts (src/MyModule/init.luau), only the init file is deleted. The directory and init.meta.json5 remain.

Fix: Detect init files and remove the parent directory entirely (since the directory IS the instance):

if instance_path.is_file() {
    let file_name = instance_path.file_name().and_then(|f| f.to_str()).unwrap_or("");
    let is_init_file = file_name.starts_with("init.");

    if is_init_file {
        // The parent directory represents the instance -- remove it all
        let dir_path = instance_path.parent().unwrap();
        self.suppress_path_remove(dir_path);
        fs::remove_dir_all(dir_path)?;
        // Also check for adjacent dir-level meta in grandparent
        // e.g., grandparent/MyModule.meta.json5
        // ... (use shared meta_path helper from DR2)
    } else {
        // Regular file: remove file + adjacent meta
        // ... (existing logic, fixed with DR2 helper for meta path)
    }
}

---

DR1. Extract shared meta name update helper

Files: src/change_processor.rs, src/web/api.rs

The pattern "check if name needs slugify, decide upsert vs remove name field, compute meta path" appears in 3+ places. Extract to a shared module.

Location: New helper functions in src/syncback/file_names.rs (or a new src/syncback/meta.rs):

• pub fn should_write_meta_name(instance_name: &str, filesystem_name: &str) -> bool -- returns true when the filesystem name differs from the instance name
• Move upsert_meta_name_field and remove_meta_name_field out of change_processor.rs into the shared module so api.rs can use them too

---

DR2. Extract shared meta_path_for_script() helper

Files: src/web/api.rs, src/change_processor.rs

Both need "given a script file path, what's the adjacent meta path?" Currently computed differently in each.

Location: src/syncback/file_names.rs:

/// Given a script file path like `parent/Foo_Bar.server.luau`,
/// returns the adjacent meta path `parent/Foo_Bar.meta.json5`.
pub fn adjacent_meta_path(script_path: &Path) -> PathBuf {
    let stem = script_path.file_stem().and_then(|s| s.to_str()).unwrap_or("");
    let base = strip_script_suffix(stem);
    script_path.with_file_name(format!("{}.meta.json5", base))
}

Both change_processor.rs and api.rs use this instead of inline computation.

---

Cleanup: Fix misleading comment

File: src/web/api.rs line 1494

Comment says "The meta file is named after the instance name, not the file name" -- this is wrong. Fix to: "The meta file is named after the script file's base stem (the slugified name), not the raw instance name."

---

Tests (7 new test scenarios)

All in tests/tests/two_way_sync.rs and tests/tests/syncback.rs / src/syncback/file_names.rs:

1. Syncback integration: forbidden chars -- Create fixture with instances named "Hey/Bro", "Key:Script". Assert slugified filenames + meta name fields.
2. Two-way sync: add with forbidden chars -- Add new sibling "A/B" during serve. Assert A_B.luau + A_B.meta.json5 with {"name": "A/B"}.
3. Two-way sync: collision -- Add "A/B" and "A:B" (both slug to A_B). Assert one gets A_B, other gets A_B~1, both with correct meta name.
4. Idempotency -- Run syncback twice on same tree. Assert zero file changes on second run.
5. Tilde dedup suffix end-to-end -- Syncback with collision producing ~1. Rebuild and verify correct names.
6. Negative: Foo~1.luau without meta -- Build fixture with Foo~1.luau (no meta). Assert instance name is Foo~1, not Foo.
7. Large tree stress -- 100+ instances with collision patterns. Assert deterministic, correct output.

---

Implementation Notes

Files created

File	Purpose
src/syncback/meta.rs	Shared upsert_meta_name() and remove_meta_name() -- pure JSON operations without filesystem event suppression. Returns RemoveNameOutcome enum so callers handle suppression themselves.
rojo-test/build-tests/tilde_no_meta/	Build fixture: Foo~1.luau with no meta file. Verifies forward sync produces instance name Foo~1.

Files modified

File	What changed
src/syncback/file_names.rs	Added KNOWN_SCRIPT_SUFFIXES, strip_script_suffix(), adjacent_meta_path(). Added 19 new unit tests (suffix stripping, meta path derivation, tilde collision, stress, idempotency).
src/syncback/mod.rs	Added pub mod meta;. Exported adjacent_meta_path. Removed deny_unknown_fields from SyncbackRules.
src/change_processor.rs	Rewrote upsert_meta_name_field / remove_meta_name_field to delegate to shared syncback::meta module. C1 fix: moved meta name update logic OUTSIDE if new_path != *path for both init-file and regular-file rename paths. Introduced effective_dir_path / effective_meta_base tracking variables.
src/web/api.rs	C2 fix: syncback_removed_instance now uses adjacent_meta_path() instead of instance_name for meta cleanup. CC2 fix: init files trigger remove_dir_all on the parent directory + grandparent dir-level meta cleanup. Removed unused instance_name variable. Fixed misleading comment.
src/snapshot_middleware/project.rs	CC1 fix: collected new_child_map.drain() into sorted Vec before processing, ensuring deterministic dedup ordering.
tests/tests/build.rs	Added tilde_no_meta to gen_build_tests! macro.
tests/tests/two_way_sync.rs	Added 2 integration tests: add_instance_with_forbidden_chars_creates_slug_and_meta, add_two_colliding_instances_deduplicates.

Test results

• 646 unit tests pass (15 new in file_names.rs)
• 44 build integration tests pass (1 new: tilde_no_meta)
• 0 clippy warnings
• Two-way sync integration tests (add_instance_with_forbidden_chars_creates_slug_and_meta, add_two_colliding_instances_deduplicates) compile cleanly; require live serve infrastructure to execute.

Key design decisions

1. **meta.rs returns outcomes, doesn't suppress** -- The shared upsert_meta_name / remove_meta_name functions are pure I/O (read JSON, modify, write/delete). Filesystem event suppression stays in the caller (change_processor.rs) because it depends on the JobThreadContext API. This keeps the shared module framework-agnostic.
2. **effective_dir_path / effective_meta_base pattern** -- For the C1 fix, rather than duplicating the meta update logic in both branches of the path-change condition, we track which path ended up being used (original or renamed) and run the meta update once afterward. This avoids the "nested inside conditional" bug class entirely.
3. **adjacent_meta_path derives from filesystem name** -- The helper strips the file extension and script suffix from the actual filesystem path, never from the instance name. This ensures the meta path always matches the script file regardless of slugification.
4. Alphabetical sort for project.rs determinism -- Chose alphabetical sort over referent-ID sort because it's human-predictable and matches what a developer would expect when looking at the filesystem.