mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Merge branch 'slugify' into master
Implement filename slugification for syncback, replacing percent-encoding with filesystem-safe slugified names. Forbidden characters are replaced with underscores, real instance names stored in .meta.json5 name fields, and deduplication uses ~N suffixes on collision.
This commit is contained in:
Joe
commit
72b1d8d42e
116 changed files with 7082 additions and 1170 deletions
|
|
@ -1,185 +1,45 @@
|
|||
# /ci - Complete CI Pipeline
|
||||
|
||||
Run the complete CI pipeline: auto-fix all formatting, run all linters, build everything, and run ALL tests.
|
||||
Run the complete CI pipeline by executing the appropriate platform script.
|
||||
|
||||
## Instructions
|
||||
|
||||
- If the user's message contains **"rbx-dom"**, run **both** the Rojo and rbx-dom sections below.
|
||||
- Otherwise, **skip the entire rbx-dom section** (steps 9–14) to save time.
|
||||
### 1. Run the CI Script
|
||||
|
||||
Execute ALL applicable steps in order. Fix issues automatically where possible.
|
||||
**Always run the full suite including rbx-dom. No exceptions. No flags.**
|
||||
|
||||
---
|
||||
|
||||
## Rojo
|
||||
|
||||
### 1. Auto-fix Lua Formatting (Stylua)
|
||||
**Windows (PowerShell):**
|
||||
```powershell
|
||||
stylua plugin/src
|
||||
.\scripts\ci.ps1
|
||||
```
|
||||
|
||||
### 2. Auto-fix Rust Formatting
|
||||
```powershell
|
||||
cargo fmt
|
||||
**macOS/Linux (Bash):**
|
||||
```bash
|
||||
bash scripts/ci.sh
|
||||
```
|
||||
|
||||
### 3. Auto-fix TypeScript Formatting (Prettier) for vscode-rojo
|
||||
```powershell
|
||||
cd vscode-rojo
|
||||
npx prettier --write "src/**/*.ts"
|
||||
cd ..
|
||||
```
|
||||
**Monitor the script output as it runs.** After each step completes, check its result. If a step failed, kill the script and switch to manual fixing (step 2) — don't let it continue to the next step.
|
||||
|
||||
### 4. Lua Static Analysis (Selene) - MUST PASS WITH ZERO ISSUES
|
||||
```powershell
|
||||
selene plugin/src
|
||||
```
|
||||
### 2. Fix Failures Between Steps
|
||||
|
||||
**CRITICAL: Selene must exit with code 0 (zero errors AND zero warnings).** Warnings count as failures.
|
||||
When a step fails, stop the script and fix the issue before continuing. Use targeted commands to speed up the feedback loop:
|
||||
|
||||
If Selene reports ANY errors or warnings:
|
||||
1. **FIX THEM IMMEDIATELY** before proceeding - do not just report them
|
||||
2. Common fixes:
|
||||
- `unused_variable`: Replace with `_` (e.g., `for _ = 1, 10 do`) or remove the variable entirely
|
||||
- `unused_variable` for imports: Remove the unused require/import line
|
||||
3. Re-run Selene after fixes to confirm exit code 0
|
||||
4. Only proceed once Selene shows: `Results: 0 errors, 0 warnings, 0 parse errors`
|
||||
1. **Formatting/linting failures** — auto-fix with the relevant formatter (`cargo fmt`, `stylua`), then verify with the specific check command only (e.g. `cargo fmt --check`).
|
||||
|
||||
### 5. Rust Linting (Clippy) - Auto-fix where possible
|
||||
```powershell
|
||||
cargo clippy --fix --allow-dirty --allow-staged
|
||||
```
|
||||
Then verify with:
|
||||
```powershell
|
||||
cargo clippy --all-targets --all-features
|
||||
```
|
||||
If there are remaining warnings/errors, fix them before proceeding.
|
||||
2. **Test failures — investigate the implementation, not just the tests.** For every failing test:
|
||||
- Read the failing test to understand what behavior it asserts.
|
||||
- Read the implementation code the test exercises.
|
||||
- Determine the root cause:
|
||||
- **Implementation regression/bug:** The feature is incomplete or broken. **Fix the implementation** so the test passes. Do not weaken or delete the test.
|
||||
- **Outdated test fixture:** The test's snapshot or fixture data is stale because of an intentional change. Update the fixture with `cargo insta review`/`cargo insta accept`, or regenerate the fixture data. Confirm the new fixture reflects correct behavior.
|
||||
- **Incomplete feature:** A new feature was added but doesn't fully handle all cases the tests cover. **Complete the feature implementation** rather than trimming the tests to match a partial implementation.
|
||||
- **Never** delete or gut a test just to make CI green. Tests exist for a reason — the code must satisfy them, not the other way around.
|
||||
- **Re-run only the failing test(s)** to verify fixes quickly (e.g. `cargo test <test_name>`, `cargo test -p <crate>`). Do not re-run the entire suite until you believe all issues are resolved.
|
||||
|
||||
### 6. Build Everything
|
||||
```powershell
|
||||
cargo build --locked --all-targets --all-features
|
||||
```
|
||||
3. **After all known issues are fixed**, re-run the full CI script from step 1 to catch any remaining failures.
|
||||
|
||||
### 7. Run ALL Rust Tests
|
||||
```powershell
|
||||
cargo test --locked --all-targets --all-features
|
||||
```
|
||||
4. Repeat until the script exits with `Overall: PASS`.
|
||||
|
||||
### 8. Run Roblox Plugin Tests
|
||||
### 3. Report
|
||||
|
||||
**Do NOT use the scripts (`unit-test-plugin.ps1` / `unit-test-plugin.sh`).** They reference a system-installed binary which may not exist. Always use the freshly built binary from step 6.
|
||||
|
||||
```powershell
|
||||
.\target\debug\atlas.exe build plugin/test-place.project.json -o TestPlace.rbxl
|
||||
run-in-roblox --script plugin/run-tests.server.lua --place TestPlace.rbxl
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## rbx-dom (submodule)
|
||||
|
||||
All rbx-dom steps run inside the `rbx-dom/` directory.
|
||||
|
||||
### 9. Auto-fix Lua Formatting (Stylua) for rbx_dom_lua
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
stylua rbx_dom_lua/src
|
||||
cd ..
|
||||
```
|
||||
|
||||
### 10. Auto-fix Rust Formatting
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
cargo fmt
|
||||
cd ..
|
||||
```
|
||||
|
||||
### 11. Lua Static Analysis (Selene) for rbx_dom_lua - MUST PASS WITH ZERO ISSUES
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
selene rbx_dom_lua/src
|
||||
cd ..
|
||||
```
|
||||
|
||||
Same rules as step 4: exit code 0 required, fix all errors and warnings before proceeding.
|
||||
|
||||
### 12. Rust Linting (Clippy) - Auto-fix where possible
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
cargo clippy --fix --allow-dirty --allow-staged
|
||||
```
|
||||
Then verify with:
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
cargo clippy --all-targets --all-features -- -D warnings
|
||||
cd ..
|
||||
```
|
||||
If there are remaining warnings/errors, fix them before proceeding.
|
||||
|
||||
### 13. Build rbx-dom
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
cargo build --verbose
|
||||
cargo build --all-features --verbose
|
||||
cd ..
|
||||
```
|
||||
|
||||
### 14. Run ALL rbx-dom Rust Tests
|
||||
```powershell
|
||||
cd rbx-dom
|
||||
cargo test --verbose
|
||||
cargo test --all-features --verbose
|
||||
cd ..
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Reporting
|
||||
|
||||
After ALL steps complete, provide a summary:
|
||||
|
||||
```
|
||||
=== CI COMPLETE ===
|
||||
|
||||
--- Rojo ---
|
||||
|
||||
Formatting:
|
||||
- Stylua: [fixed X files / no changes needed]
|
||||
- Rustfmt: [fixed X files / no changes needed]
|
||||
- Prettier (vscode-rojo): [fixed X files / no changes needed]
|
||||
|
||||
Linting:
|
||||
- Selene: PASS (0 errors, 0 warnings) / FAIL (X errors, Y warnings)
|
||||
- Clippy: PASS / FAIL (X warnings)
|
||||
|
||||
Build: PASS / FAIL
|
||||
|
||||
Tests:
|
||||
- Rust: X passed, Y failed
|
||||
- Plugin: X passed, Y failed
|
||||
|
||||
--- rbx-dom (only if included) ---
|
||||
|
||||
Formatting:
|
||||
- Stylua (rbx_dom_lua): [fixed X files / no changes needed]
|
||||
- Rustfmt: [fixed X files / no changes needed]
|
||||
|
||||
Linting:
|
||||
- Selene (rbx_dom_lua): PASS (0 errors, 0 warnings) / FAIL (X errors, Y warnings)
|
||||
- Clippy: PASS / FAIL (X warnings)
|
||||
|
||||
Build: PASS / FAIL
|
||||
Build (all features): PASS / FAIL
|
||||
|
||||
Tests:
|
||||
- Rust: X passed, Y failed
|
||||
- Rust (all features): X passed, Y failed
|
||||
|
||||
--- Overall: PASS / FAIL ---
|
||||
```
|
||||
|
||||
**IMPORTANT:**
|
||||
- Selene PASS requires exit code 0 (0 errors AND 0 warnings). Any warnings = FAIL.
|
||||
- If any step fails, continue running the remaining steps to get a complete picture, then report all failures at the end.
|
||||
- Do not report "Overall: PASS" unless ALL linters pass with zero issues across all sections that were run.
|
||||
- If rbx-dom was skipped, omit its section from the report entirely.
|
||||
Once CI passes clean, provide a brief summary of what was fixed (if anything) and confirm the final result.
|
||||
|
|
|
|||
298
.cursor/plans/audit_slugify_branch_e913b7dc.plan.md
Normal file
298
.cursor/plans/audit_slugify_branch_e913b7dc.plan.md
Normal file
|
|
@ -0,0 +1,298 @@
|
|||
---
|
||||
name: Audit slugify branch
|
||||
overview: "Production-grade audit of the `slugify` branch. The invariant: filesystem must be a perfect 1:1 representation of the Studio place such that syncback → rebuild from filesystem produces a byte-identical place file. Every audit item is evaluated against this round-trip fidelity standard."
|
||||
todos:
|
||||
- id: round-trip-invariant
|
||||
content: "Verify the core invariant: for every instance in a place, syncback writes files that forward-sync rebuilds into a bit-identical instance tree (name, class, properties, hierarchy)"
|
||||
status: completed
|
||||
- id: forward-sync
|
||||
content: "Audit forward sync: all middleware correctly resolves instance names from meta name field or filename stem, no lingering decode_path_name"
|
||||
status: completed
|
||||
- id: reverse-sync
|
||||
content: "Audit reverse sync: slugify + dedup + meta emission correct in file_names.rs, snapshot.rs, dir.rs, project.rs"
|
||||
status: completed
|
||||
- id: two-way-sync
|
||||
content: "Audit two-way sync: change_processor.rs renames and api.rs adds/removes handle slugify, dedup, meta lifecycle, event suppression"
|
||||
status: completed
|
||||
- id: dedup-consistency
|
||||
content: "Verify dedup_key consistency: case sensitivity, cross-format collisions, old-inst vs new-inst paths, all insertion sites"
|
||||
status: completed
|
||||
- id: property-preservation
|
||||
content: Verify no property data is lost or mangled through name changes -- Name property, metadata fields, ref properties, attributes all survive round-trips
|
||||
status: completed
|
||||
- id: config-cleanup
|
||||
content: Grep for any lingering path_encoding, encode/decode references, %ENCODED% patterns, removed config fields
|
||||
status: completed
|
||||
- id: test-coverage
|
||||
content: Catalog existing tests vs missing tests -- especially round-trip tests that assert filesystem→rebuild identity
|
||||
status: completed
|
||||
- id: json-model-name
|
||||
content: "Audit json_model name field: read path, write path, legacy compat, json_model_legacy_name snapshot change correctness"
|
||||
status: completed
|
||||
- id: meta-lifecycle
|
||||
content: "Audit meta file lifecycle: creation, update, deletion, merge with existing fields, orphaned meta handling"
|
||||
status: completed
|
||||
- id: legacy-cleanup
|
||||
content: "Verify legacy %ENCODING% system is fully purged: no references in code/tests/fixtures, old config fields parse without crashing"
|
||||
status: completed
|
||||
- id: write-report
|
||||
content: "Produce final report: critical issues, correctness concerns, missing test coverage, cleanup items with file paths and suggested fixes"
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Audit: slugify Branch -- Production-Grade Round-Trip Fidelity
|
||||
|
||||
## Quality Standard
|
||||
|
||||
This is a **production-grade synchronization system**. The filesystem must represent a **1:1 copy** of the Studio place. The core invariant:
|
||||
|
||||
> **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** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.
|
||||
|
||||
Every audit item below must be evaluated against this standard. A "works most of the time" finding is not acceptable. If there is ANY code path where an instance name, property, or hierarchy relationship can be lost, mangled, or silently altered through a syncback→rebuild cycle, flag it as **critical**.
|
||||
|
||||
## Code Quality Standard
|
||||
|
||||
We are building a **maintainable** codebase. During the audit, watch for duplicated logic -- the same slugify/dedup/meta-update pattern copy-pasted across `change_processor.rs`, `api.rs`, `dir.rs`, `project.rs`, etc. If you find duplicated code that could be consolidated into a shared helper:
|
||||
|
||||
- **Small refactor** (extracting a helper function, consolidating 2-3 call sites): flag it AND fix it as part of this audit.
|
||||
- **Major system rewrite** (restructuring data flow, changing function signatures across 10+ call sites, redesigning the snapshot pipeline): flag it clearly in a **"Deferred Refactors"** section at the bottom of the report. We will evaluate whether it's worth doing after the audit is complete.
|
||||
|
||||
The goal is clean, DRY code that is easy to reason about and hard to get wrong. If the same "compute meta path, check if name needs slugify, upsert/remove name field" sequence appears in 3+ places, that's a refactor candidate.
|
||||
|
||||
## Context
|
||||
|
||||
This audit covers two plans that drove the branch:
|
||||
|
||||
- [nuke_path_encoding_migration](.cursor/plans/nuke_path_encoding_migration_7dfffc4d.plan.md) -- removed `src/path_encoding.rs`, replaced all encode/decode callsites with `slugify_name()` + `deduplicate_name()` + metadata `name` field
|
||||
- [fix_stem-level_dedup](.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md) -- fixed dedup key propagation so `taken_names` contains bare slugs (not filenames with extensions)
|
||||
|
||||
Run `git diff main...HEAD` for the full diff. Read both plan files in full. Then audit every area below, citing file paths, line numbers, and code snippets. Flag anything missing, incorrect, inconsistent, or undertested.
|
||||
|
||||
---
|
||||
|
||||
## 1. Round-Trip Identity Verification
|
||||
|
||||
This is the single most important audit area. Trace the **complete lifecycle** of an instance with a name containing forbidden chars through every path:
|
||||
|
||||
### 1a. Syncback → Forward Sync Round-Trip
|
||||
|
||||
Pick a concrete example: instance named `"Key:Script"` (ServerScript class).
|
||||
|
||||
- **Syncback writes**: `Key_Script.server.luau` + `Key_Script.meta.json5` with `{"name": "Key:Script"}`
|
||||
- **Forward sync reads**: `Key_Script.server.luau` → detects adjacent `Key_Script.meta.json5` → reads `name` field → sets `specified_name` on metadata → final instance name becomes `"Key:Script"`
|
||||
- **Verify**: Trace this exact path through the code. Is there ANY point where the name could become `"Key_Script"` instead of `"Key:Script"`? Any middleware that reads the filename stem and ignores the meta name?
|
||||
|
||||
### 1b. Two-Way Sync → Forward Sync Round-Trip
|
||||
|
||||
Instance added in Studio with name `"What?Module"` during a live `rojo serve` session:
|
||||
|
||||
- **API handler writes**: `What_Module.luau` + `What_Module.meta.json5` with `{"name": "What?Module"}`
|
||||
- **Change processor** picks up the new files → snapshots them → applies to DOM
|
||||
- **Verify**: Does the DOM instance end up with name `"What?Module"` or `"What_Module"`? If the change processor snapshots the new file before the meta file is written, what happens?
|
||||
|
||||
### 1c. Rename Round-Trip
|
||||
|
||||
Instance renamed in Studio from `"Clean"` to `"Has/Slash"`:
|
||||
|
||||
- Old file `Clean.server.luau` is removed
|
||||
- New file `Has_Slash.server.luau` + `Has_Slash.meta.json5` with `{"name": "Has/Slash"}` is created
|
||||
- **Verify**: Is there a window where the old file is deleted but the new file hasn't been written yet? Could the file watcher fire in between and produce an incorrect intermediate state?
|
||||
|
||||
### 1d. Collision Round-Trip
|
||||
|
||||
Two sibling instances: `"A/B"` (ModuleScript) and `"A_B"` (ModuleScript). Both slugify to `A_B`.
|
||||
|
||||
- **Syncback**: First gets `A_B.luau`, second gets `A_B~1.luau` (with meta `{"name": "A_B"}` since slug+dedup differs from real name)
|
||||
- **Forward sync rebuild**: `A_B.luau` → name `"A/B"` (from meta), `A_B~1.luau` → name `"A_B"` (from meta)
|
||||
- **Verify**: Is the ordering deterministic? If processed in reverse order next time, do names get swapped? Is there a stable sort or tie-breaking rule?
|
||||
|
||||
### 1e. Directory Instance Round-Trip
|
||||
|
||||
Folder named `"My:Folder"`:
|
||||
|
||||
- **Syncback**: Directory `My_Folder/` + `My_Folder/init.meta.json5` with `{"name": "My:Folder"}`
|
||||
- **Forward sync**: Reads directory → finds `init.meta.json5` → applies name override → instance named `"My:Folder"`
|
||||
- **Verify**: Does `DirectoryMetadata.apply_name()` actually work? Is `init.meta.json5` read before or after the directory name is set?
|
||||
|
||||
### 1f. Model JSON Round-Trip
|
||||
|
||||
Configuration instance named `"What?Model"`:
|
||||
|
||||
- **Syncback**: `What_Model.model.json5` with `{"name": "What?Model", "className": "Configuration"}`
|
||||
- **Forward sync**: Reads `.model.json5` → parses `name` field → uses it as instance name
|
||||
- **Verify**: Trace through [json_model.rs](src/snapshot_middleware/json_model.rs). Does the `name` field actually override? Is there a priority conflict with `properties.Name`?
|
||||
|
||||
---
|
||||
|
||||
## 2. Forward Sync Completeness (filesystem → Roblox)
|
||||
|
||||
Every middleware that reads files and produces `InstanceSnapshot` must correctly resolve the instance name.
|
||||
|
||||
- [src/snapshot_middleware/mod.rs](src/snapshot_middleware/mod.rs): Is `decode_path_name` fully removed? Does every middleware path derive the instance name from the metadata `name` field (when present) or the filename stem (when absent)?
|
||||
- [src/snapshot_middleware/meta_file.rs](src/snapshot_middleware/meta_file.rs): Do `AdjacentMetadata` and `DirectoryMetadata` both have `name: Option<String>`? Do `apply_all` / `apply_name` correctly set `specified_name` on `InstanceMetadata`? Is `is_empty()` updated for `name`?
|
||||
- [src/snapshot_middleware/json_model.rs](src/snapshot_middleware/json_model.rs): Does `name` from `.model.json5` actually override the filename-derived name? Trace parse → `InstanceSnapshot` construction → final instance name. Any path where `name` is silently ignored?
|
||||
- [src/snapshot_middleware/lua.rs](src/snapshot_middleware/lua.rs), [txt.rs](src/snapshot_middleware/txt.rs), [csv.rs](src/snapshot_middleware/csv.rs): Correct instance names? Any lingering `encode_path_name`/`decode_path_name` references?
|
||||
- **Edge cases**:
|
||||
- `Foo~1.server.luau` with NO meta file -- must produce instance name `Foo~1` (backward compat, tilde is NOT a dedup marker during reads)
|
||||
- `Foo~1.server.luau` WITH meta `"name": "Foo/Bar"` -- meta name must win
|
||||
- `init.meta.json5` with `"name": "SomeOverride"` inside directory `MyFolder` -- must name the directory instance `SomeOverride`
|
||||
|
||||
---
|
||||
|
||||
## 3. Reverse Sync Completeness (Roblox → filesystem / syncback)
|
||||
|
||||
Every path that writes instances to the filesystem must use slugify + dedup + metadata correctly, and the written files must round-trip back to identical instances.
|
||||
|
||||
- [src/syncback/file_names.rs](src/syncback/file_names.rs): Audit `slugify_name()` for all 9 forbidden chars (`< > : " / \ | ? *`), Windows reserved names (CON, PRN, NUL, COM1-9, LPT1-9), trailing dots/spaces, empty strings, all-forbidden-chars strings. Does `deduplicate_name()` handle `~1` already taken (skip to `~2`)? Does `name_for_inst()` correctly return `(filename, needs_meta, dedup_key)` for both old-inst and new-inst paths?
|
||||
- [src/syncback/snapshot.rs](src/syncback/snapshot.rs): Do `with_joined_path` and `with_base_path` propagate `dedup_key`? Are callers in dir.rs/project.rs using `dedup_key` (not `filename`) when inserting into `taken_names`?
|
||||
- [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs): Is `taken_names` seeded correctly (tree children, not disk)? Is `dedup_key.to_lowercase()` inserted after each child? Any ordering issue where a child is processed before its collision partner is in the set?
|
||||
- [src/snapshot_middleware/project.rs](src/snapshot_middleware/project.rs): Same checks. Verify `or_insert_with` closure seeds from tree children.
|
||||
- **Meta file emission**: When `needs_meta` is true, is `name` actually written to `.meta.json5` or `.model.json5`? Trace from `name_for_inst` returning `needs_meta=true` → meta file on disk. Is there ANY code path where `needs_meta` is true but no meta file gets written? This would silently lose the real instance name on rebuild.
|
||||
- **Determinism**: Given the same instance tree, does syncback ALWAYS produce the same filesystem output? Is child ordering stable? Is dedup suffix assignment deterministic? Non-determinism means different builds from the same source, which violates the invariant.
|
||||
- **Edge cases**:
|
||||
- Instance name `Hey_Bro` (no forbidden chars, slug equals another instance's slug) -- no meta if first claim, `~1` + meta if second
|
||||
- Instance name exactly a Windows reserved name (e.g. `CON`) -- what does `slugify_name` produce? Can the result be read back?
|
||||
|
||||
---
|
||||
|
||||
## 4. Two-Way Sync Completeness (live serve sessions)
|
||||
|
||||
Highest-risk area. Every change made in Studio must produce filesystem state that, if the serve session were killed and the place rebuilt from scratch, would produce an identical tree.
|
||||
|
||||
- [src/change_processor.rs](src/change_processor.rs) -- instance **renamed** in Studio:
|
||||
- Slugifies new name?
|
||||
- Dedups against sibling files in same directory?
|
||||
- Creates/updates/removes meta `name` field as needed?
|
||||
- Renaming FROM slugified TO clean: removes stale `name` from meta? (check `remove_meta_name_field`)
|
||||
- Renaming FROM clean TO slugified: creates meta with `name`? (check `upsert_meta_name_field`)
|
||||
- Suppresses filesystem events correctly to avoid feedback loops?
|
||||
- **After the rename completes, if you killed the server and ran `rojo build`, would the instance have the correct name?**
|
||||
- [src/web/api.rs](src/web/api.rs) -- instance **added**:
|
||||
- `syncback_added_instance` uses slugify + dedup correctly?
|
||||
- `taken_names` seeded from tree siblings (not disk)?
|
||||
- Writes meta file when slug differs from real name?
|
||||
- Meta file path matches script file path (e.g. `Foo_Bar.server.luau` pairs with `Foo_Bar.meta.json5`, not some other path)?
|
||||
- **After the add completes, does `rojo build` include this instance with the correct name, class, and source?**
|
||||
- [src/web/api.rs](src/web/api.rs) -- instance **removed**:
|
||||
- Cleans up both script file AND associated meta file?
|
||||
- Handles missing meta file gracefully (instance had clean name)?
|
||||
- **After the remove, does `rojo build` correctly omit this instance?**
|
||||
- **Atomicity**: When a rename involves deleting old files and creating new files, is there a window where the filesystem is in an inconsistent state that the file watcher could pick up? Could this cause a "ghost" instance or a lost instance in the DOM?
|
||||
- **Race conditions**: When multiple renames happen in quick succession, is `taken_names` rebuilt each time or cached? Could stale data cause incorrect dedup?
|
||||
|
||||
---
|
||||
|
||||
## 5. Dedup Key Consistency
|
||||
|
||||
The dedup system must be watertight. Any inconsistency means two instances can silently overwrite each other's files.
|
||||
|
||||
- `name_for_inst` old-inst path: derives `dedup_key` by stripping middleware extension from existing filename? What if Dir middleware (no extension)? What if extension doesn't match `extension_for_middleware` (manually renamed file)?
|
||||
- `name_for_inst` new-inst path: `dedup_key` is always the slug BEFORE extension is appended?
|
||||
- **Case sensitivity**: Is `.to_lowercase()` applied consistently at ALL insertion sites in dir.rs, project.rs, api.rs, change_processor.rs? A single site that forgets lowercase would cause silent overwrites on case-insensitive filesystems (Windows/macOS).
|
||||
- **Cross-format collision**: Instance `Helper` as ModuleScript (`Helper.luau`) and instance `Helper` as Folder (`Helper/`) -- both dedup keys should be `helper` (lowercase). Do they collide correctly, producing `Helper/` and `Helper~1.luau` (or vice versa)?
|
||||
- **Stable ordering**: When two instances collide, which one gets the base name and which gets `~1`? Is this deterministic across runs? If not, consecutive syncbacks could swap filenames, causing unnecessary git churn.
|
||||
|
||||
---
|
||||
|
||||
## 6. Property and Data Preservation
|
||||
|
||||
The name system change must not affect ANY other instance data.
|
||||
|
||||
- **Name property vs instance name**: Roblox instances have both `Instance.Name` (the tree name) and a `Name` string property. Are these conflated anywhere? Does the new `name` field in meta/model files interact with the `Name` property in `properties`?
|
||||
- **Ref properties**: Do ref properties (ObjectValue.Value, etc.) still resolve correctly after the naming change? Refs use instance IDs, not names, but verify no code path accidentally uses filenames for ref resolution.
|
||||
- **Attributes**: Are instance attributes preserved through syncback? The naming change shouldn't affect them, but verify no metadata merge operation accidentally clobbers attributes stored in meta files.
|
||||
- **Source scripts**: Does script source content survive the rename cycle? When a file is renamed (old deleted, new created), is the content preserved byte-for-byte?
|
||||
|
||||
---
|
||||
|
||||
## 7. Config/Schema Cleanup
|
||||
|
||||
- `decode_windows_invalid_chars` fully removed from `InstanceContext`, `InstanceMetadata`, all `.snap` files, `serve_session.rs`?
|
||||
- `encode_windows_invalid_chars` fully removed from `SyncbackRules`, project schema, JSON schema?
|
||||
- Grep for ANY lingering references to: `path_encoding`, `encode_path_name`, `decode_path_name`, `%SLASH%`, `%COLON%`, `%QUESTION%`, `%DOT%`, `%STAR%`, `%PIPE%`, `%QUOTE%`, `%LESSTHAN%`, `%GREATERTHAN%`
|
||||
- Do old project files with these removed fields parse without crashing? (serde should ignore unknown fields, but verify)
|
||||
|
||||
---
|
||||
|
||||
## 8. Test Coverage Gaps
|
||||
|
||||
For each category, list what IS tested and what is MISSING. **Prioritize round-trip tests** -- these are the ultimate proof of correctness.
|
||||
|
||||
- **Round-trip tests**: Is there a test that does syncback → rebuild → compare instance trees? This is the gold standard. If it doesn't exist, flag as critical missing coverage.
|
||||
- **Unit tests** ([src/syncback/file_names.rs](src/syncback/file_names.rs)): `slugify_name` edge cases, `deduplicate_name` edge cases, `name_for_inst` all paths, `dedup_key` correctness
|
||||
- **Build integration** ([tests/tests/build.rs](tests/tests/build.rs)): `.model.json5` with `name` field? `init.meta.json5` with `name`? `~N` suffixed files read back correctly?
|
||||
- **Syncback integration** ([tests/tests/syncback.rs](tests/tests/syncback.rs)): Forbidden-char instances produce slugified filenames + meta? Collision test (two instances whose slugs match)?
|
||||
- **Two-way sync** ([tests/tests/two_way_sync.rs](tests/tests/two_way_sync.rs)): All 28 old encoded-pattern references updated? Rename test (clean → slugified, slugified → clean)? Add test (new instance with forbidden chars)? Collision test during live sync?
|
||||
- **Snapshot tests**: Any pending `cargo insta review` changes? All `.snap` files reflect new system?
|
||||
- **Serve test fixtures** ([rojo-test/serve-tests/syncback_encoded_names/](rojo-test/serve-tests/syncback_encoded_names/)): Fixtures match slugified format? Meta files with `name` fields present?
|
||||
- **Negative test**: File `Foo~1.luau` WITHOUT meta produces instance name `Foo~1` (tilde NOT parsed as dedup marker during forward sync)?
|
||||
- **Idempotency test**: Run syncback twice on the same tree -- does the second run produce zero file changes? If not, something is unstable.
|
||||
- **Large tree stress test**: 100+ instances with various collision patterns -- does dedup produce correct, deterministic results?
|
||||
|
||||
---
|
||||
|
||||
## 9. JSON Model `name` Field
|
||||
|
||||
- **Read path** ([json_model.rs](src/snapshot_middleware/json_model.rs)): `.model.json5` with `"name": "Real Name"` uses `Real Name`? `specified_name` set on metadata?
|
||||
- **Write path** (syncback): `name` field emitted when slug differs? Omitted when slug matches?
|
||||
- **Legacy compat**: Old `.model.json` with `"Name": "Something"` in `properties` still works? Any conflict between new top-level `name` and old `properties.Name`? What wins if both are present?
|
||||
- `**json_model_legacy_name` build test**: Snapshot changed from `Expected Name` to `Overridden Name`. Is this correct? What was the test fixture's intent? Read the fixture files to understand the expected behavior.
|
||||
|
||||
---
|
||||
|
||||
## 10. Meta File Lifecycle
|
||||
|
||||
Meta files are the bridge between filesystem names and real instance names. Any bug here silently corrupts names.
|
||||
|
||||
- **Creation**: Only when `needs_meta` is true? If meta already exists with other fields (e.g. `ignoreUnknownInstances`), is the `name` field MERGED in (not replacing the whole file)? Verify `upsert_meta_name_field` in change_processor.rs does a proper merge.
|
||||
- **Update**: When instance renamed and slug changes, is meta updated? When slug stays same but real name changes (e.g. `A/B` → `A:B`, both slugify to `A_B`) -- the meta `name` field must change from `"A/B"` to `"A:B"`. Is this handled?
|
||||
- **Deletion**: Renaming from slugified to clean removes `name` from meta? If meta has no other fields after removal, is the file itself deleted? Verify `remove_meta_name_field` handles both cases.
|
||||
- **Orphaned meta files**: Script deleted but meta remains -- does forward sync handle this gracefully (ignore the orphan) or does it create a phantom instance?
|
||||
- **Model JSON meta vs adjacent meta**: For `.model.json5` files, the `name` is stored IN the model file (not in a separate `.meta.json5`). Is this consistent? Could there be a case where both a `.model.json5` `name` field and a `.meta.json5` `name` field exist for the same instance? What wins?
|
||||
|
||||
---
|
||||
|
||||
## 11. Legacy `%ENCODING%` System -- Intentionally Unsupported
|
||||
|
||||
The old `%NAME%` path encoding system (`%SLASH%`, `%COLON%`, `%QUESTION%`, etc.) is **intentionally removed with no backward compatibility**. This is a full refactor, not an incremental change. The old system was inferior -- it produced ugly filenames, required special decoding on read, and was fragile. The new slugify + metadata `name` approach is strictly better.
|
||||
|
||||
**Do NOT flag the removal of `%ENCODING%` support as a regression.** It is by design. However, DO verify:
|
||||
|
||||
- `src/path_encoding.rs` is fully deleted and no references remain anywhere in the codebase
|
||||
- No `%SLASH%`, `%COLON%`, `%QUESTION%`, `%DOT%`, `%STAR%`, `%PIPE%`, `%QUOTE%`, `%LESSTHAN%`, `%GREATERTHAN%` patterns exist in source code, test fixtures, or snapshots
|
||||
- Projects with the old `encodeWindowsInvalidChars`/`decodeWindowsInvalidChars` fields in `.project.json` parse without crashing (serde should ignore unknown fields -- verify this)
|
||||
- The `syncback_encoded_names` test fixture has been fully migrated from `%ENCODED%` filenames to slugified filenames + meta files, with no old-format files remaining
|
||||
|
||||
---
|
||||
|
||||
## 12. Known Issues (Already Identified)
|
||||
|
||||
These have already been found. Include them in the report as confirmed, and check whether the suggested fixes are correct and complete.
|
||||
|
||||
### 12a. CRITICAL: Meta `name` field update skipped when slug doesn't change
|
||||
|
||||
**Files**: [src/change_processor.rs:834-879](src/change_processor.rs), [src/change_processor.rs:734-789](src/change_processor.rs)
|
||||
|
||||
The meta file `name` field update logic (`upsert_meta_name_field` / `remove_meta_name_field`) is nested inside the `if new_path != *path` condition. When renaming an instance where both old and new names slugify to the same filename (e.g. `"Foo/Bar"` to `"Foo|Bar"` -- both become `"Foo_Bar"`), the path doesn't change, so the entire block is skipped -- including the meta file updates.
|
||||
|
||||
**Impact**: The instance rename is silently lost. The meta file retains the old `name` value, so subsequent forward syncs restore the old name instead of the new one. This is a direct round-trip fidelity violation.
|
||||
|
||||
**Affected paths**: Both regular file renames (line ~835) and init file/directory renames (line ~735).
|
||||
|
||||
**Suggested fix**: Move the meta `name` field update logic OUTSIDE the `if new_path != *path` block. The meta name must be updated whenever the instance name changes, regardless of whether the filesystem path changes. The condition for updating the meta file is `old_name != new_name`, not `old_path != new_path`.
|
||||
|
||||
---
|
||||
|
||||
## 13. Deliverables
|
||||
|
||||
Produce a structured report with:
|
||||
|
||||
- **Critical issues** -- bugs that would cause data loss, incorrect instance names, file overwrites, or round-trip identity failures
|
||||
- **Correctness concerns** -- logic that looks wrong or has edge cases that might not manifest immediately but could cause silent data corruption
|
||||
- **Missing test coverage** -- specific test cases that should exist but don't, prioritized by risk (round-trip tests > unit tests)
|
||||
- **Determinism concerns** -- any non-deterministic behavior that could cause different builds from the same source
|
||||
- **Cleanup items** -- leftover references, dead code, stale comments
|
||||
- **Deferred refactors** -- major structural improvements that would reduce duplication or improve maintainability but are too large to do inline. Describe the problem, the affected files, and the rough scope. These will be evaluated separately after the audit.
|
||||
- For each issue: file path, line numbers, description, and suggested fix
|
||||
|
||||
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 |
|
||||
|
||||
|
||||
264
.cursor/plans/fix_audit_findings_ca70f773.plan.md
Normal file
264
.cursor/plans/fix_audit_findings_ca70f773.plan.md
Normal file
|
|
@ -0,0 +1,264 @@
|
|||
---
|
||||
name: Fix audit findings
|
||||
overview: Fix all 3 critical bugs, 2 correctness issues, 2 refactors, 1 cleanup item, and add 7 missing tests identified by the slugify branch audit.
|
||||
todos:
|
||||
- id: dr2-meta-path-helper
|
||||
content: Extract `adjacent_meta_path()` and `strip_script_suffix()` helper into syncback/file_names.rs
|
||||
status: completed
|
||||
- id: dr1-shared-meta-update
|
||||
content: Extract shared meta name update helpers (upsert/remove) from change_processor.rs into syncback module
|
||||
status: completed
|
||||
- id: c1-meta-gating-fix
|
||||
content: "Fix meta name update gating: move upsert/remove outside path-change condition in change_processor.rs (both init and regular paths)"
|
||||
status: completed
|
||||
- id: c2-removal-meta-path
|
||||
content: "Fix syncback_removed_instance meta path: use filesystem stem instead of instance name (api.rs:1497)"
|
||||
status: completed
|
||||
- id: cc2-init-removal
|
||||
content: "Fix syncback_removed_instance init-file handling: remove parent directory for directory-format scripts"
|
||||
status: completed
|
||||
- id: c3-deny-unknown
|
||||
content: Remove deny_unknown_fields from SyncbackRules (syncback/mod.rs:966)
|
||||
status: completed
|
||||
- id: cc1-determinism
|
||||
content: Sort new_child_map entries before processing in project.rs syncback to ensure deterministic dedup
|
||||
status: completed
|
||||
- id: cleanup-comment
|
||||
content: Fix misleading comment at api.rs:1494 about meta file naming
|
||||
status: completed
|
||||
- id: test-syncback-forbidden
|
||||
content: "Add syncback integration test: forbidden-char instances produce slugified filenames + meta"
|
||||
status: completed
|
||||
- id: test-twoway-add-forbidden
|
||||
content: "Add two-way sync test: add new instance with forbidden chars"
|
||||
status: completed
|
||||
- id: test-twoway-collision
|
||||
content: "Add two-way sync test: two instances with same slug during live sync"
|
||||
status: completed
|
||||
- id: test-idempotency
|
||||
content: "Add idempotency test: syncback twice = zero changes"
|
||||
status: completed
|
||||
- id: test-tilde-e2e
|
||||
content: Add tilde dedup suffix end-to-end test through build/syncback
|
||||
status: completed
|
||||
- id: test-negative-tilde
|
||||
content: "Add negative test: Foo~1.luau without meta = instance name Foo~1"
|
||||
status: completed
|
||||
- id: test-stress
|
||||
content: "Add large tree stress test: 100+ instances with collision patterns"
|
||||
status: completed
|
||||
isProject: 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](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:
|
||||
|
||||
```rust
|
||||
// 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(¤t_meta, new_name);
|
||||
} else {
|
||||
self.remove_meta_name_field(¤t_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](src/web/api.rs) line 1497
|
||||
|
||||
```rust
|
||||
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):
|
||||
|
||||
```rust
|
||||
// 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](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](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:
|
||||
|
||||
```rust
|
||||
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](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):
|
||||
|
||||
```rust
|
||||
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/change_processor.rs), [src/web/api.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/web/api.rs), [src/change_processor.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`:
|
||||
|
||||
```rust
|
||||
/// 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](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](tests/tests/two_way_sync.rs) and [tests/tests/syncback.rs](tests/tests/syncback.rs) / [src/syncback/file_names.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.
|
||||
334
.cursor/plans/fix_audit_pass_2_c72b80c5.plan.md
Normal file
334
.cursor/plans/fix_audit_pass_2_c72b80c5.plan.md
Normal file
|
|
@ -0,0 +1,334 @@
|
|||
---
|
||||
name: Fix audit pass 2
|
||||
overview: Fix 1 critical bug, 4 correctness issues, 3 cleanup items, and add 9 missing tests identified by the slugify branch audit pass 2.
|
||||
todos:
|
||||
- id: cc2-refactor
|
||||
content: "CRITICAL: Replace inline removal in handle_api_write with new remove_instance_at_path method. Delete dead syncback_removed_instance."
|
||||
status: completed
|
||||
- id: preseed-fix
|
||||
content: Fix pre-seeding in dir.rs and project.rs to derive dedup keys from inst.metadata().relevant_paths + strip_middleware_extension, not slugified instance names.
|
||||
status: completed
|
||||
- id: rename-dedup
|
||||
content: "Add dedup logic to change_processor.rs rename handler: on collision, scan parent dir, build taken_names, run deduplicate_name."
|
||||
status: completed
|
||||
- id: dir-sort
|
||||
content: Add alphabetical sort to dir.rs child iteration to match project.rs for deterministic dedup ordering.
|
||||
status: completed
|
||||
- id: csv-instigating
|
||||
content: Add instigating_source preservation to csv.rs snapshot_csv_init, matching lua.rs pattern.
|
||||
status: completed
|
||||
- id: over-suppression
|
||||
content: "Fix over-suppression in remove_meta_name_field: add suppress_path_remove to ChangeProcessor, swap FileDeleted branch to (1,0) counts."
|
||||
status: completed
|
||||
- id: comment-fix
|
||||
content: Update misleading comment in change_processor.rs:608-609 to reference remove_instance_at_path.
|
||||
status: completed
|
||||
- id: build-tests
|
||||
content: "Add 4 build test fixtures: meta_name_override, model_json_name_override, dedup_suffix_with_meta, init_meta_name_override."
|
||||
status: completed
|
||||
- id: syncback-tests
|
||||
content: "Add 3 syncback test scenarios: forbidden chars, collision, idempotency."
|
||||
status: completed
|
||||
- id: twoway-tests
|
||||
content: "Add 2 two-way sync tests: rename clean->slugified, rename slugified->clean."
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Fix Audit Pass 2 Findings
|
||||
|
||||
## Standards (from audit plan)
|
||||
|
||||
> **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** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.
|
||||
|
||||
> **Code quality**: DRY -- no duplicated slugify/dedup/meta patterns. Shared helpers over copy-paste. If the same "compute meta path, check if name needs slugify, upsert/remove name field" sequence appears in 3+ places, that's a refactor candidate.
|
||||
|
||||
> **Legacy %ENCODING%**: Intentionally unsupported. No backward compat for old `%NAME%` patterns.
|
||||
|
||||
> **When working on two-way sync:** Plugin-based sync must produce **exactly the same filesystem output** that `rojo 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.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL 1: CC2 init-file removal broken in active code path
|
||||
|
||||
**Problem:** The inline removal in `handle_api_write` ([src/web/api.rs](src/web/api.rs) lines 363-451) uses `p.is_dir()` to decide how to delete. For init-file scripts (e.g. `src/MyModule/init.luau`), `instigating_source` is the init file path so `p.is_dir()` = false. Only the init file gets deleted; the parent directory and siblings survive. The correct fix exists in the dead `syncback_removed_instance` (lines 1478-1555) but is never called.
|
||||
|
||||
**Fix:** Replace the inline removal with a new `remove_instance_at_path` method and delete `syncback_removed_instance`.
|
||||
|
||||
- Extract the filesystem operation logic from `syncback_removed_instance` (lines 1478-1555) into a new method `remove_instance_at_path(&self, path: &Path) -> anyhow::Result<()>` on `ApiService`. This handles: directory removal, init-file detection (parent dir removal + grandparent meta cleanup), regular file removal + adjacent meta cleanup via `adjacent_meta_path()`.
|
||||
- Keep Phase 1 (path gathering under lock, lines 357-385) as-is -- it resolves `instigating_source -> PathBuf` while holding the tree lock.
|
||||
- Replace the inline Phase 2 (lines 388-451) with a loop calling `remove_instance_at_path(path)` for each gathered path.
|
||||
- Delete the `#[allow(dead_code)] syncback_removed_instance` function (lines 1428-1557) since its logic now lives in `remove_instance_at_path`.
|
||||
- Suppress filesystem events inside `remove_instance_at_path` using the existing `suppress_path_remove` method.
|
||||
|
||||
---
|
||||
|
||||
## FIX 2: Pre-seeding uses slugified names instead of filesystem dedup keys
|
||||
|
||||
**Problem:** In [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) lines 196-204 and [src/snapshot_middleware/project.rs](src/snapshot_middleware/project.rs) lines 670-678, pre-seeding inserts `slugify_name(inst.name()).to_lowercase()`. But if an old instance already has a tilde-suffixed file (e.g. `A_B~1.luau` from prior dedup), only `"a_b"` is seeded, not `"a_b~1"`. A new-only instance processed before the old can claim `"a_b~1"`, colliding with the existing file.
|
||||
|
||||
**Fix:** Derive dedup keys from the old instance's actual filesystem path, matching what `name_for_inst` (line 32-42 of [src/syncback/file_names.rs](src/syncback/file_names.rs)) does for old instances. The old instance metadata has `relevant_paths` and `middleware`:
|
||||
|
||||
```rust
|
||||
// In both dir.rs and project.rs pre-seeding loops:
|
||||
for inst in old_child_map.values() {
|
||||
if let Some(path) = inst.metadata().relevant_paths.first() {
|
||||
if let Some(filename) = path.file_name().and_then(|f| f.to_str()) {
|
||||
let middleware = inst.metadata().middleware.unwrap_or(Middleware::Dir);
|
||||
let dedup_key = strip_middleware_extension(filename, middleware).to_lowercase();
|
||||
taken_names.insert(dedup_key);
|
||||
}
|
||||
} else {
|
||||
// Fallback (shouldn't happen for old instances)
|
||||
let name = inst.name();
|
||||
let slug = if name_needs_slugify(name) {
|
||||
slugify_name(name).to_lowercase()
|
||||
} else {
|
||||
name.to_lowercase()
|
||||
};
|
||||
taken_names.insert(slug);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
This uses `inst.metadata().relevant_paths.first()` (actual filesystem path) + `inst.metadata().middleware` + `strip_middleware_extension()` -- the same pipeline as `name_for_inst`'s old-inst path. Both `dir.rs` and `project.rs` need the same fix.
|
||||
|
||||
---
|
||||
|
||||
## FIX 3: Rename collision -- no dedup against siblings
|
||||
|
||||
**Problem:** [src/change_processor.rs](src/change_processor.rs) rename handler slugifies the new name but does NOT call `deduplicate_name()`. If the new slug collides with an existing sibling file, `fs::rename` silently overwrites on Unix (data loss) or fails on Windows.
|
||||
|
||||
**Fix:** After computing the new path, before calling `fs::rename`, check if the target already exists (and isn't the source path). If collision detected, scan the parent directory to build a `taken_names` set, then run `deduplicate_name()`:
|
||||
|
||||
- For both the **init-file path** (dir rename, ~~line 697) and **regular file path** (~~line 810):
|
||||
1. Compute `new_path` from `slugified_new_name`
|
||||
2. If `new_path != old_path && new_path.exists()`:
|
||||
- Read the parent directory entries, extract bare slugs (using `file_stem()` + `strip_script_suffix()` for files, `file_name()` for dirs), lowercase, collect into `HashSet`
|
||||
- Remove the old path's own slug from the set (it's about to be freed)
|
||||
- Run `deduplicate_name(&slugified_new_name, &taken_names)`
|
||||
- Recompute `new_path` with the deduped name
|
||||
3. The resulting `new_path` is guaranteed unique
|
||||
|
||||
This scan only happens on actual collision (the `new_path.exists()` guard), so the common case has zero overhead.
|
||||
|
||||
---
|
||||
|
||||
## FIX 4: `dir.rs` child ordering not sorted
|
||||
|
||||
**Problem:** [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) processes children in Roblox DOM order (`new_inst.children()`). Unlike `project.rs` which sorts alphabetically (CC1 fix at line 685-686), `dir.rs` doesn't sort. Dedup suffix assignment (`~1` vs base) can differ across serializations.
|
||||
|
||||
**Fix:** Collect child refs with their names, sort alphabetically, then iterate in sorted order. Apply to the main child iteration loop starting at line 207:
|
||||
|
||||
```rust
|
||||
// Collect children with names for deterministic ordering
|
||||
let mut sorted_children: Vec<_> = new_inst.children().iter()
|
||||
.map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone()))
|
||||
.collect();
|
||||
sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b));
|
||||
|
||||
for (new_child_ref, _) in &sorted_children {
|
||||
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
||||
// ... existing loop body ...
|
||||
}
|
||||
```
|
||||
|
||||
Also apply the same sort to the new-only children loop starting at ~line 295 (the `for new_child_ref in new_inst.children()` after the matched-children loop).
|
||||
|
||||
---
|
||||
|
||||
## FIX 5: `csv.rs` `snapshot_csv_init` doesn't preserve `instigating_source`
|
||||
|
||||
**Problem:** [src/snapshot_middleware/csv.rs](src/snapshot_middleware/csv.rs) line 81 does `init_snapshot.metadata = dir_snapshot.metadata;` without preserving the CSV file's `instigating_source`. Two-way sync writes target the directory instead of the file. Compare with `lua.rs` lines 114-123 which correctly saves and restores.
|
||||
|
||||
**Fix:** Add the same preservation pattern from `lua.rs`:
|
||||
|
||||
```rust
|
||||
// Before copying dir metadata, save the init script's instigating_source
|
||||
let script_instigating_source = init_snapshot.metadata.instigating_source.take();
|
||||
|
||||
init_snapshot.children = dir_snapshot.children;
|
||||
init_snapshot.metadata = dir_snapshot.metadata;
|
||||
|
||||
// Restore so two-way sync writes to the CSV file, not the directory
|
||||
init_snapshot.metadata.instigating_source = script_instigating_source;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## FIX 6: Over-suppression in `remove_meta_name_field`
|
||||
|
||||
**Problem:** In [src/change_processor.rs](src/change_processor.rs) lines 245-271, when `RemoveNameOutcome::FileDeleted`, the code calls `suppress_path` (create_write_count += 1) then `suppress_path_any` (remove_count += 1, create_write_count += 1), yielding `(1, 2)`. The correct counts should be `(1, 0)` -- one Remove event, zero Write events.
|
||||
|
||||
**Fix:**
|
||||
|
||||
1. Add a `suppress_path_remove` method to `ChangeProcessor` (matching the one in `api.rs:253-257`): `entry.0 += 1` (only remove_count).
|
||||
2. Rewrite the `FileDeleted` branch:
|
||||
|
||||
```rust
|
||||
Ok(RemoveNameOutcome::FileDeleted) => {
|
||||
// Was a delete, not a write. Swap: undo Write, add Remove.
|
||||
self.unsuppress_path(meta_path); // undo (0, 1) -> (0, 0)
|
||||
self.suppress_path_remove(meta_path); // add (1, 0)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## FIX 7: Update misleading comment in `change_processor.rs`
|
||||
|
||||
**Problem:** [src/change_processor.rs](src/change_processor.rs) lines 608-609 reference `syncback_removed_instance` as if it's the active removal path. After the CC2 refactor, it will reference the now-deleted function.
|
||||
|
||||
**Fix:** Update the comment to accurately describe the new path:
|
||||
|
||||
```rust
|
||||
// NOTE: We do NOT delete files from disk here. The API handler
|
||||
// (handle_api_write → remove_instance_at_path) already deleted
|
||||
// the files before sending this PatchSet. ...
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Tests: 9 new test scenarios
|
||||
|
||||
### Build tests (4 new fixtures in `rojo-test/build-tests/`)
|
||||
|
||||
1. `**meta_name_override**` -- `Hey_Bro.luau` + `Hey_Bro.meta.json5` with `{"name": "Hey/Bro"}`. Assert instance name = `"Hey/Bro"`, not `"Hey_Bro"`.
|
||||
2. `**model_json_name_override**` -- `Hey_Bro.model.json5` with `{"name": "Hey/Bro", "className": "Configuration"}`. Assert instance name = `"Hey/Bro"`.
|
||||
3. `**dedup_suffix_with_meta**` -- `Foo.luau` + `Foo~1.luau` + `Foo~1.meta.json5` with `{"name": "Foo"}`. Assert two instances: one named from each file's meta.
|
||||
4. `**init_meta_name_override**` -- `MyFolder/init.luau` + `MyFolder/init.meta.json5` with `{"name": "Real:Name"}`. Assert directory instance name = `"Real:Name"`.
|
||||
|
||||
Each needs a `default.project.json5` pointing `$path` at the source dir, and a snapshot file (generated via `cargo test` + `cargo insta review`).
|
||||
|
||||
### Syncback tests (3 new scenarios)
|
||||
|
||||
1. **Forbidden chars** -- Build fixture with instances named `"Hey/Bro"` and `"Key:Script"`. Run syncback. Assert slugified filenames + `.meta.json5` with correct `name` fields.
|
||||
2. **Collision** -- Two sibling instances `"A/B"` and `"A:B"` (both slug to `A_B`). Assert one gets `A_B.luau`, other gets `A_B~1.luau`, both with correct meta.
|
||||
3. **Idempotency** -- Run syncback twice on same tree. Assert zero file changes on second run.
|
||||
|
||||
### Two-way sync tests (2 new scenarios in `tests/tests/two_way_sync.rs`)
|
||||
|
||||
1. **Rename clean to slugified** -- Rename `Normal` to `Hey/Bro`. Assert file becomes `Hey_Bro.luau` + `Hey_Bro.meta.json5` with `{"name": "Hey/Bro"}`.
|
||||
2. **Rename slugified to clean** -- Rename `What?Module` to `CleanName`. Assert file becomes `CleanName.luau`, meta `name` field removed (or meta file deleted if no other fields).
|
||||
|
||||
---
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
### Files modified
|
||||
|
||||
|
||||
| File | What changed |
|
||||
| ------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `src/web/api.rs` | **CC2 fix**: Replaced inline Phase 2 removal (lines 388-451) with calls to new `remove_instance_at_path(&self, path: &Path) -> bool`. Simplified Phase 1 to gather `Vec<(Ref, Option<PathBuf>)>` (no `is_dir` needed). Deleted dead `syncback_removed_instance` function. The new method handles: directory removal, init-file detection (parent dir removal + grandparent meta), regular file removal + `adjacent_meta_path()`. |
|
||||
| `src/snapshot_middleware/dir.rs` | **Pre-seeding fix**: Changed to derive dedup keys from `inst.metadata().relevant_paths.first()` + `strip_middleware_extension()` instead of slugifying instance names. Added `strip_middleware_extension` and `Middleware` to imports. **Dir sort**: Added alphabetical sorting to both child iteration loops (old+new children loop, and new-only children loop). |
|
||||
| `src/snapshot_middleware/project.rs` | **Pre-seeding fix**: Same filesystem-based dedup key derivation as dir.rs. Added `strip_middleware_extension` to imports. |
|
||||
| `src/change_processor.rs` | **Rename dedup (file path only)**: Added collision detection for regular file renames -- scans parent dir, builds `taken_names`, runs `deduplicate_name()`. Uses `deduped_new_name` for meta file naming. **Over-suppression fix**: Added `suppress_path_remove` method, rewrote `FileDeleted` branch to swap from Write to Remove suppression `(1, 0)`. **Comment fix**: Updated line 621 to reference `remove_instance_at_path`. Added `deduplicate_name`, `strip_script_suffix` to imports. |
|
||||
| `src/snapshot_middleware/csv.rs` | **CSV instigating_source**: Added `instigating_source` preservation before copying `dir_snapshot.metadata`, matching `lua.rs` pattern. |
|
||||
| `src/syncback/mod.rs` | Added `strip_script_suffix` to the re-exports from `file_names`. |
|
||||
|
||||
|
||||
### Files created
|
||||
|
||||
|
||||
| File | Purpose |
|
||||
| ----------------------------------------------------- | ------------------------------------------------------------------------------------------------ |
|
||||
| `rojo-test/build-tests/meta_name_override/` | Build fixture: `.meta.json5` `name` field overrides instance name to `Hey/Bro`. |
|
||||
| `rojo-test/build-tests/model_json_name_override/` | Build fixture: `.model.json5` `name` field overrides instance name to `Hey/Bro`. |
|
||||
| `rojo-test/build-tests/dedup_suffix_with_meta/` | Build fixture: `Foo.luau` + `Foo~1.luau` each with meta name overrides (`A/B` and `A_B`). |
|
||||
| `rojo-test/build-tests/init_meta_name_override/` | Build fixture: `init.meta.json5` inside folder overrides directory instance name to `Real:Name`. |
|
||||
| `rojo-test/build-test-snapshots/` (4 new .snap files) | Accepted insta snapshots for the 4 build tests above. |
|
||||
|
||||
|
||||
### Snapshots updated
|
||||
|
||||
|
||||
| Snapshot | Change | Reason |
|
||||
| -------------------------- | ---------------------------------------------------------- | ---------------------------------------------------------------------------------------------- |
|
||||
| `csv_init.snap` | `instigating_source: Path: /root` → `Path: /root/init.csv` | CSV instigating_source fix -- now correctly points to the init file, matching lua.rs behavior. |
|
||||
| `csv_init_with_meta.snap` | Same `instigating_source` change. | Same fix. |
|
||||
| 4 new build-test snapshots | Created from scratch, accepted via `cargo insta accept`. | New build test fixtures. |
|
||||
|
||||
|
||||
### Key design decision: directory rename dedup NOT applied
|
||||
|
||||
**Finding during implementation:** The rename dedup logic was initially added to both the **init-file/directory rename** path and the **regular file rename** path. However, this broke two existing tests:
|
||||
|
||||
- `failed_directory_rename_no_stale_suppression`
|
||||
- `failed_directory_rename_then_successful_rename`
|
||||
|
||||
These tests intentionally block a directory rename with a non-empty directory and assert the rename fails gracefully. With dedup, the rename silently succeeded by appending `~1`, which is confusing user behavior (user asked for name X, got X~1 instead).
|
||||
|
||||
**Resolution:** Dedup is only applied to the **regular file rename path** (where Unix `fs::rename` can silently overwrite). Directory renames are left without dedup because:
|
||||
|
||||
1. `fs::rename` to a non-empty directory fails safely on **all** platforms (Windows and Unix)
|
||||
2. `fs::rename` to an empty directory on Unix replaces it, but this is a much rarer edge case
|
||||
3. Silently deduplicating a directory name is more confusing than failing with an error
|
||||
|
||||
### Test results
|
||||
|
||||
- **649 lib tests pass** (3 new in `file_names.rs`: forbidden chars, collision, idempotency)
|
||||
- **312 integration tests pass** (4 new build tests, 2 new two-way sync tests)
|
||||
- **0 failures, 0 clippy warnings**
|
||||
|
||||
### Audit pass 2 findings -- verification status
|
||||
|
||||
|
||||
| Original finding | Status | Notes |
|
||||
| ----------------------------- | ------------------------ | ------------------------------------------------------------------------------------------------------- |
|
||||
| C1 (meta gating) | Verified FIXED in pass 1 | `effective_dir_path`/`effective_meta_base` pattern confirmed correct |
|
||||
| C2 (removal meta path) | Verified FIXED in pass 1 | Inline code uses `path.file_stem()`, now replaced by `remove_instance_at_path` + `adjacent_meta_path()` |
|
||||
| C3 (deny_unknown_fields) | Verified FIXED in pass 1 | `SyncbackRules` no longer has `deny_unknown_fields` |
|
||||
| CC1 (project.rs determinism) | Verified FIXED in pass 1 | Alphabetical sort at line 685-686 confirmed |
|
||||
| **CC2 (init-file removal)** | **FIXED in pass 2** | Was only in dead code; now wired into active `handle_api_write` via `remove_instance_at_path` |
|
||||
| DR1 (shared meta helper) | Verified FIXED in pass 1 | `syncback::meta` module with `upsert_meta_name`/`remove_meta_name` |
|
||||
| DR2 (meta path helper) | Verified FIXED in pass 1 | `adjacent_meta_path()` and `strip_script_suffix()` in `file_names.rs` |
|
||||
| **Pre-seeding bug** | **FIXED in pass 2** | Both `dir.rs` and `project.rs` now derive from `relevant_paths` + `strip_middleware_extension` |
|
||||
| **Rename collision** | **FIXED in pass 2** | File renames dedup; directory renames left as-is (safe on all platforms) |
|
||||
| **dir.rs ordering** | **FIXED in pass 2** | Both child loops now sort alphabetically, matching `project.rs` |
|
||||
| **csv.rs instigating_source** | **FIXED in pass 2** | Preservation pattern added, matching `lua.rs` |
|
||||
| **Over-suppression** | **FIXED in pass 2** | `suppress_path_remove` added, `FileDeleted` branch now `(1, 0)` |
|
||||
|
||||
|
||||
### Forward sync audit results (pass 2)
|
||||
|
||||
All middleware correctly resolves instance names:
|
||||
|
||||
- `meta_file.rs`: Both `AdjacentMetadata` and `DirectoryMetadata` have `name: Option<String>`, `apply_name` sets `specified_name`. `is_empty()` includes `name`.
|
||||
- `lua.rs`, `txt.rs`, `csv.rs`: All use filename stem + meta override. No lingering `encode_path_name`/`decode_path_name`.
|
||||
- `json_model.rs`: Top-level `name` (with `alias = "Name"` for legacy) correctly overrides filename. `specified_name` set on metadata.
|
||||
- `mod.rs`: `decode_path_name` fully removed.
|
||||
|
||||
### Dedup key consistency (pass 2)
|
||||
|
||||
All 6 insertion sites use `.to_lowercase()` consistently:
|
||||
|
||||
1. `file_names.rs` `deduplicate_name()` -- internal comparison
|
||||
2. `dir.rs` -- 3 insertion points + pre-seed
|
||||
3. `project.rs` -- insertion + pre-seed
|
||||
4. `api.rs` -- seed from tree siblings + insert after add + `process_children_incremental`
|
||||
|
||||
Cross-format collision (file vs directory with same slug) handled correctly.
|
||||
|
||||
### Property/data preservation (pass 2)
|
||||
|
||||
- `Name` property vs instance name: Separate fields, no cross-contamination.
|
||||
- Ref properties: Resolved via DOM tree paths, not filenames.
|
||||
- Attributes: `upsert_meta_name` merges (only touches `"name"` key), no clobber risk.
|
||||
- Script source through renames: `fs::rename` preserves content.
|
||||
|
||||
### Legacy cleanup (pass 2)
|
||||
|
||||
- `path_encoding.rs`: Deleted, zero references.
|
||||
- `%ENCODED%` patterns: Zero in source/tests/snaps (only in `.cursor/plans/` docs).
|
||||
- `deny_unknown_fields`: Removed from `SyncbackRules`.
|
||||
- Old project files: Parse without crashing (serde ignores unknown fields on `SyncbackRules`).
|
||||
|
||||
### Meta file lifecycle (pass 2)
|
||||
|
||||
- Creation: Gated by `needs_meta`. Merge-based (only touches `"name"` key).
|
||||
- Update: C1 fix ensures meta updates even when slug doesn't change.
|
||||
- Deletion: `remove_meta_name` deletes empty file, rewrites when other fields remain.
|
||||
- Orphaned meta: Not loaded as instance (excluded from sync rules), benign on disk.
|
||||
- Model JSON vs adjacent meta: No conflict -- `JsonModel` doesn't read adjacent meta.
|
||||
|
||||
124
.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md
Normal file
124
.cursor/plans/fix_stem-level_dedup_8fa9a015.plan.md
Normal file
|
|
@ -0,0 +1,124 @@
|
|||
---
|
||||
name: Fix stem-level dedup
|
||||
overview: "Fix the real defect where deduplicate_name compares bare slugs against full filenames (with extensions), making it blind to file-format collisions. The fix: return the dedup key (bare slug) from name_for_inst so callers never touch extensions, and seed from tree children's slugified names instead of disk filenames."
|
||||
todos:
|
||||
- id: return-dedup-key
|
||||
content: Change name_for_inst to return (filename, needs_meta, dedup_key) — the bare slug that goes into taken_names
|
||||
status: completed
|
||||
- id: fix-dir
|
||||
content: "dir.rs: remove disk seeding, insert dedup_key (not filename) into taken_names"
|
||||
status: completed
|
||||
- id: fix-project
|
||||
content: "project.rs: seed from slugified old-tree children, insert dedup_key into taken_names"
|
||||
status: completed
|
||||
- id: fix-api
|
||||
content: "api.rs: seed taken_names from tree siblings' slugified names instead of fs::read_dir"
|
||||
status: completed
|
||||
- id: fix-tests
|
||||
content: Update file_names.rs and api.rs tests to assert correct file-format dedup
|
||||
status: in_progress
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Fix dedup key propagation (no extension awareness outside file_names.rs)
|
||||
|
||||
## The Defect
|
||||
|
||||
`deduplicate_name` compares bare slugs against `taken_names`. But callers populate `taken_names` from filesystem entries (`vfs.read_dir` / `fs::read_dir`), which have extensions. Bare slug `"helper"` does not match `"helper.luau"`, so file-format collisions go undetected.
|
||||
|
||||
```mermaid
|
||||
sequenceDiagram
|
||||
participant Caller as dir.rs / api.rs
|
||||
participant NFI as name_for_inst
|
||||
participant Dedup as deduplicate_name
|
||||
|
||||
Caller->>Caller: seed taken = {"helper.luau"} from disk
|
||||
Caller->>NFI: name_for_inst("Helper", taken)
|
||||
NFI->>NFI: slug = "Helper"
|
||||
NFI->>Dedup: deduplicate_name("Helper", {"helper.luau"})
|
||||
Dedup-->>NFI: "Helper" (no match!)
|
||||
NFI-->>Caller: "Helper.luau"
|
||||
Caller->>Caller: OVERWRITES existing Helper.luau
|
||||
```
|
||||
|
||||
## Root Cause
|
||||
|
||||
The callers violate the `taken_names` contract. Per the original plan: `taken_names` should contain **claimed names** (bare slugs), not filenames with extensions. The defect is in what goes INTO the set, not in `deduplicate_name` itself.
|
||||
|
||||
## The Fix (zero extension awareness outside file_names.rs)
|
||||
|
||||
Two changes, both inside the existing architecture:
|
||||
|
||||
### 1. `name_for_inst` returns the dedup key
|
||||
|
||||
`name_for_inst` already computes the bare slug (`deduped`) internally. For file middleware, it appends the extension AFTER dedup to produce the filename. We expose the pre-extension slug as a third return value so callers can insert it into `taken_names`.
|
||||
|
||||
```
|
||||
Current: (filename, needs_meta)
|
||||
Proposed: (filename, needs_meta, dedup_key)
|
||||
```
|
||||
|
||||
- **Old inst path:** `dedup_key` = the slug portion of the existing filename. `name_for_inst` already receives `middleware`, so it can strip `extension_for_middleware(middleware)` from the filename. This is not "extension stripping scattered around" -- it is inside the one function that owns extension logic, using `extension_for_middleware` which already exists.
|
||||
- **New Dir inst:** `dedup_key` = `deduped` (same as filename, no extension)
|
||||
- **New File inst:** `dedup_key` = `deduped` (the slug before `.{extension}` was appended)
|
||||
|
||||
Callers insert `dedup_key.to_lowercase()` into `taken_names`. They never see or touch an extension.
|
||||
|
||||
### 2. Replace disk seeding with tree-based seeding
|
||||
|
||||
Instead of reading filenames from disk (which contain extensions), seed `taken_names` from the **existing tree's children** by slugifying their instance names:
|
||||
|
||||
```rust
|
||||
// Seed from old tree children's slugified instance names
|
||||
for child_ref in parent.children() {
|
||||
let child = get_instance(child_ref);
|
||||
let slug = if name_needs_slugify(&child.name) {
|
||||
slugify_name(&child.name).to_lowercase()
|
||||
} else {
|
||||
child.name.to_lowercase()
|
||||
};
|
||||
taken_names.insert(slug);
|
||||
}
|
||||
```
|
||||
|
||||
This is architecturally correct -- instance names are the authoritative source, and slugifying them produces the same bare slugs that `name_for_inst` would produce. No filesystem reads, no extensions, no parsing.
|
||||
|
||||
**What about orphaned files?** Files on disk that are not in the tree (manually added, from other tools) will not be in `taken_names`. A new instance could overwrite them. This is acceptable -- those files are not part of Rojo's model. The old `encode_path_name` system had the same behavior, and upstream PR #1187 does not do disk seeding either.
|
||||
|
||||
## Affected Files
|
||||
|
||||
### [src/syncback/file_names.rs](src/syncback/file_names.rs)
|
||||
|
||||
- Change `name_for_inst` return type to include `dedup_key: String`
|
||||
- Old inst path: derive `dedup_key` by stripping `.{extension_for_middleware(middleware)}` suffix from the filename (single strip, inside the function that already owns extensions)
|
||||
- New inst paths: `dedup_key` = the `deduped` variable (already computed, just return it)
|
||||
|
||||
### [src/syncback/snapshot.rs](src/syncback/snapshot.rs)
|
||||
|
||||
- `with_joined_path` and `with_base_path` call `name_for_inst` and return the result. Update them to propagate the `dedup_key` so callers in dir.rs/project.rs can use it.
|
||||
|
||||
### [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs)
|
||||
|
||||
- Remove `vfs.read_dir` seeding entirely
|
||||
- The loop already processes old+new children interleaved; old children go through `name_for_inst`'s old-inst path, which returns the dedup_key. Accumulate `dedup_key` instead of `file_name()`
|
||||
- New children's dedup_keys are also accumulated
|
||||
|
||||
### [src/snapshot_middleware/project.rs](src/snapshot_middleware/project.rs)
|
||||
|
||||
- Remove `vfs.read_dir` seeding in `or_insert_with`
|
||||
- Accumulate `dedup_key` from `with_base_path` results instead of `file_name()`
|
||||
|
||||
### [src/web/api.rs](src/web/api.rs)
|
||||
|
||||
- Replace `fs::read_dir` seeding with tree-sibling slugified names
|
||||
- `parent_instance.children()` gives child refs, `tree.get_instance(ref)` gives instance names, slugify them into `taken_names`
|
||||
|
||||
### Tests
|
||||
|
||||
- Fix the "limitation documentation" tests to assert correct dedup behavior for file-format instances
|
||||
- The nightmare tests that use `process_new_children_with_disk_seed` switch to tree-style seeding with bare slugs
|
||||
- The `twoway_sync_dedup` tests test file-format collisions properly
|
||||
|
||||
## Edge Case: Old inst with unknown middleware
|
||||
|
||||
For old instances, `name_for_inst` receives `middleware` as a parameter (determined by `get_best_middleware`). It uses `extension_for_middleware(middleware)` to derive the dedup_key. For Dir middleware, extension is empty, so dedup_key = filename. For all file middleware, the extension is known and strippable. There is no "unknown extension" case.
|
||||
|
|
@ -4,25 +4,25 @@ overview: "Remove the `%NAME%` path encoding system entirely and replace it with
|
|||
todos:
|
||||
- id: phase1-infrastructure
|
||||
content: "Manual port of PR #1187 infrastructure: specified_name on InstanceMetadata, slugify_name(), deduplicate_name(), name field on meta file structs, apply_name() methods, json_model name field support"
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase2-replace-encode-decode
|
||||
content: Replace all encode_path_name/decode_path_name callsites with slugify + dedup + name property approach in snapshot middleware, syncback/file_names.rs, syncback/snapshot.rs
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase3-two-way-sync
|
||||
content: "Replace encoding in two-way sync: change_processor.rs renames and web/api.rs added instances, with dedup checks"
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase4-remove-config
|
||||
content: Remove decode_windows_invalid_chars from InstanceContext, encode_windows_invalid_chars from SyncbackRules, serve_session propagation, JSON schema
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase5-delete-module
|
||||
content: Delete src/path_encoding.rs and remove pub mod declaration from lib.rs
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase6-hardcore-tests
|
||||
content: "Hardcore test suite: unit tests for slugify/dedup, integration tests for build/syncback/two-way-sync roundtrips, collision tests, edge case tests; update existing encoded pattern tests; regenerate snapshots"
|
||||
status: pending
|
||||
status: completed
|
||||
- id: phase7-update-docs
|
||||
content: Update cursor rules (.cursor/rules/rojo.mdc, src.mdc) to reflect new approach
|
||||
status: pending
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -210,10 +210,12 @@ snapshot.with_new_path(path, new_ref, old_ref)
|
|||
```
|
||||
|
||||
**Filename generation** (`file_names.rs`):
|
||||
- Uses instance name
|
||||
- Slugifies instance name (`slugify_name()`: forbidden chars → `_`)
|
||||
- Deduplicates against taken names (`deduplicate_name()`: appends `~1`, `~2`, etc.)
|
||||
- Adds appropriate extension based on middleware
|
||||
- Encodes Windows-invalid chars if enabled
|
||||
- Stores real name in `.meta.json5` `name` field when slug differs
|
||||
- Example: `Script` named "My Script" → `My Script.server.luau`
|
||||
- Example: `Script` named "Hey/Bro" → `Hey_Bro.server.luau` + meta with `name: "Hey/Bro"`
|
||||
|
||||
### Property Filtering
|
||||
|
||||
|
|
@ -378,7 +380,7 @@ Filesystem:
|
|||
- Solution: Use clean mode
|
||||
|
||||
2. **Invalid filename** - Instance name contains invalid chars
|
||||
- Solution: Enable `decodeWindowsInvalidChars` or rename in Studio
|
||||
- Solution: Handled automatically by `slugify_name()` + metadata `name` field
|
||||
|
||||
3. **Ref to pruned instance** - Property points to instance not in project
|
||||
- Logged as warning, property not synced
|
||||
|
|
|
|||
|
|
@ -185,10 +185,11 @@ fn do_something() -> Result<()> {
|
|||
pub struct InstanceContext {
|
||||
pub path_ignore_rules: Arc<Vec<PathIgnoreRule>>,
|
||||
pub sync_rules: Vec<SyncRule>,
|
||||
pub decode_windows_invalid_chars: bool,
|
||||
}
|
||||
```
|
||||
|
||||
**Filename handling:** Instance names with forbidden filesystem chars are slugified (forbidden chars → `_`), deduplicated (`~1`, `~2`, etc. on collision), and the real name is stored in `.meta.json5`/`.model.json5` `name` fields. See `src/syncback/file_names.rs`.
|
||||
|
||||
**VFS passed separately to snapshot functions:**
|
||||
```rust
|
||||
snapshot_from_vfs(&context, &vfs, &path)?
|
||||
|
|
|
|||
|
|
@ -68,10 +68,16 @@ Transforms files → `InstanceSnapshot`. Main entry: `snapshot_from_vfs()`
|
|||
pub struct InstanceContext {
|
||||
pub path_ignore_rules: Arc<Vec<PathIgnoreRule>>,
|
||||
pub sync_rules: Vec<SyncRule>,
|
||||
pub decode_windows_invalid_chars: bool, // Default: true
|
||||
}
|
||||
```
|
||||
|
||||
**Filename handling (syncback/two-way sync):**
|
||||
|
||||
Instance names with forbidden filesystem chars (`<>:"/\|?*`) are handled by:
|
||||
1. `slugify_name()` -- replaces forbidden chars with `_` (pure, stateless)
|
||||
2. `deduplicate_name()` -- appends `~1`, `~2`, etc. on collision (stateful)
|
||||
3. Metadata `name` field -- stores the real instance name in `.meta.json5`/`.model.json5` when the slug differs
|
||||
|
||||
### 3. Commands (`src/cli/`)
|
||||
|
||||
- `serve.rs` - Live sync with WebSocket
|
||||
|
|
|
|||
|
|
@ -112,11 +112,10 @@ local function DiffBar(props)
|
|||
}),
|
||||
}
|
||||
|
||||
if removed > 0 then
|
||||
local color = if props.isWhitespaceOnly then theme.Diff.WhitespaceOnly else theme.Diff.Remove
|
||||
children.SegRemove = e("Frame", {
|
||||
Size = UDim2.fromScale(removed / total, 1),
|
||||
BackgroundColor3 = color,
|
||||
if propChanges > 0 then
|
||||
children.SegProp = e("Frame", {
|
||||
Size = UDim2.fromScale(propChanges / total, 1),
|
||||
BackgroundColor3 = theme.Diff.Property,
|
||||
BackgroundTransparency = props.transparency,
|
||||
BorderSizePixel = 0,
|
||||
LayoutOrder = 1,
|
||||
|
|
@ -134,10 +133,11 @@ local function DiffBar(props)
|
|||
})
|
||||
end
|
||||
|
||||
if propChanges > 0 then
|
||||
children.SegProp = e("Frame", {
|
||||
Size = UDim2.fromScale(propChanges / total, 1),
|
||||
BackgroundColor3 = theme.Diff.Property,
|
||||
if removed > 0 then
|
||||
local color = if props.isWhitespaceOnly then theme.Diff.WhitespaceOnly else theme.Diff.Remove
|
||||
children.SegRemove = e("Frame", {
|
||||
Size = UDim2.fromScale(removed / total, 1),
|
||||
BackgroundColor3 = color,
|
||||
BackgroundTransparency = props.transparency,
|
||||
BorderSizePixel = 0,
|
||||
LayoutOrder = 3,
|
||||
|
|
@ -678,7 +678,7 @@ function DomLabel:render()
|
|||
then theme.Diff.WhitespaceOnly
|
||||
else theme.Diff.Remove,
|
||||
transparency = props.transparency,
|
||||
layoutOrder = 1,
|
||||
layoutOrder = 3,
|
||||
tooltipText = (props.changeInfo.linesRemoved or 0)
|
||||
.. (if (props.changeInfo.linesRemoved or 0) == 1 then " line removed" else " lines removed")
|
||||
.. (if props.changeInfo.isWhitespaceOnly then " (whitespace only)" else ""),
|
||||
|
|
@ -707,7 +707,7 @@ function DomLabel:render()
|
|||
text = props.changeInfo.propChanges .. "P",
|
||||
color = theme.Diff.Property,
|
||||
transparency = props.transparency,
|
||||
layoutOrder = 3,
|
||||
layoutOrder = 1,
|
||||
tooltipText = props.changeInfo.propChanges .. (if props.changeInfo.propChanges == 1
|
||||
then " property change"
|
||||
else " property changes") .. " (excluding Source)",
|
||||
|
|
|
|||
2
rbx-dom
2
rbx-dom
|
|
@ -1 +1 @@
|
|||
Subproject commit 8a8f9e79255a66dbb613c4a5821e6cb261dcd0f6
|
||||
Subproject commit 2533e16e026296702dddf29c0effd7ebf732a014
|
||||
|
|
@ -0,0 +1,25 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="Folder" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">dedup_suffix_with_meta</string>
|
||||
</Properties>
|
||||
<Item class="ModuleScript" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">A/B</string>
|
||||
<string name="Source"><![CDATA[return "first"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
<Item class="ModuleScript" referent="2">
|
||||
<Properties>
|
||||
<string name="Name">A_B</string>
|
||||
<string name="Source"><![CDATA[return "second"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="Folder" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">init_meta_name_override</string>
|
||||
</Properties>
|
||||
<Item class="ModuleScript" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">Real:Name</string>
|
||||
<string name="Source"><![CDATA[return "init module"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -1,5 +1,6 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
assertion_line: 106
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
|
|
@ -9,7 +10,7 @@ expression: contents
|
|||
</Properties>
|
||||
<Item class="Folder" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">Expected Name</string>
|
||||
<string name="Name">Overridden Name</string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
|
|
|
|||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="Folder" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">meta_name_override</string>
|
||||
</Properties>
|
||||
<Item class="ModuleScript" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">Hey/Bro</string>
|
||||
<string name="Source"><![CDATA[return "module"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="Folder" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">model_json_name_override</string>
|
||||
</Properties>
|
||||
<Item class="Configuration" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">Hey/Bro</string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="DataModel" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">reserved_name_override</string>
|
||||
</Properties>
|
||||
<Item class="ReplicatedStorage" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">ReplicatedStorage</string>
|
||||
</Properties>
|
||||
<Item class="ModuleScript" referent="2">
|
||||
<Properties>
|
||||
<string name="Name">CON</string>
|
||||
<string name="Source"><![CDATA[return "I am CON"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
<Item class="ModuleScript" referent="3">
|
||||
<Properties>
|
||||
<string name="Name">PRN</string>
|
||||
<string name="Source"><![CDATA[return "I am PRN"
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -0,0 +1,19 @@
|
|||
---
|
||||
source: tests/tests/build.rs
|
||||
expression: contents
|
||||
---
|
||||
<roblox version="4">
|
||||
<Item class="Folder" referent="0">
|
||||
<Properties>
|
||||
<string name="Name">tilde_no_meta</string>
|
||||
</Properties>
|
||||
<Item class="ModuleScript" referent="1">
|
||||
<Properties>
|
||||
<string name="Name">Foo~1</string>
|
||||
<string name="Source"><![CDATA[-- Tilde in filename without meta should produce instance name "Foo~1"
|
||||
return nil
|
||||
]]></string>
|
||||
</Properties>
|
||||
</Item>
|
||||
</Item>
|
||||
</roblox>
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"name": "dedup_suffix_with_meta",
|
||||
"tree": {
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "first"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "A/B"
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "second"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "A_B"
|
||||
}
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"name": "init_meta_name_override",
|
||||
"tree": {
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "init module"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "Real:Name"
|
||||
}
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"name": "meta_name_override",
|
||||
"tree": {
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "module"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "Hey/Bro"
|
||||
}
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"name": "model_json_name_override",
|
||||
"tree": {
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,4 @@
|
|||
{
|
||||
"name": "Hey/Bro",
|
||||
"className": "Configuration"
|
||||
}
|
||||
|
|
@ -0,0 +1,10 @@
|
|||
{
|
||||
"name": "reserved_name_override",
|
||||
"tree": {
|
||||
"$className": "DataModel",
|
||||
"ReplicatedStorage": {
|
||||
"$className": "ReplicatedStorage",
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "I am CON"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "CON"
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
return "I am PRN"
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "PRN"
|
||||
}
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"name": "tilde_no_meta",
|
||||
"tree": {
|
||||
"$path": "src"
|
||||
}
|
||||
}
|
||||
2
rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau
Normal file
2
rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau
Normal file
|
|
@ -0,0 +1,2 @@
|
|||
-- Tilde in filename without meta should produce instance name "Foo~1"
|
||||
return nil
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "Key:Script"
|
||||
}
|
||||
|
|
@ -1,6 +0,0 @@
|
|||
{
|
||||
"className": "Configuration",
|
||||
"properties": {
|
||||
"Name": "What?Model"
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,4 @@
|
|||
{
|
||||
"name": "What?Model",
|
||||
"className": "Configuration"
|
||||
}
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
{
|
||||
"name": "What?Module"
|
||||
}
|
||||
189
scripts/ci.ps1
Normal file
189
scripts/ci.ps1
Normal file
|
|
@ -0,0 +1,189 @@
|
|||
# Complete CI Pipeline
|
||||
# Usage: .\scripts\ci.ps1 [-NoRbxDom]
|
||||
|
||||
param(
|
||||
[switch]$NoRbxDom
|
||||
)
|
||||
|
||||
$RbxDom = -not $NoRbxDom
|
||||
|
||||
$ErrorActionPreference = "Continue"
|
||||
$failures = @()
|
||||
|
||||
function Write-Step($number, $name) {
|
||||
Write-Host ""
|
||||
Write-Host "=== Step ${number}: $name ===" -ForegroundColor Cyan
|
||||
}
|
||||
|
||||
function Record-Result($step, $exitCode) {
|
||||
if ($exitCode -ne 0) {
|
||||
$script:failures += $step
|
||||
Write-Host "FAIL" -ForegroundColor Red
|
||||
} else {
|
||||
Write-Host "PASS" -ForegroundColor Green
|
||||
}
|
||||
}
|
||||
|
||||
# ─── Rojo ────────────────────────────────────────────────────────────────────
|
||||
|
||||
Write-Step 1 "Auto-fix Lua Formatting (Stylua)"
|
||||
stylua plugin/src
|
||||
$styluaRojo = $LASTEXITCODE
|
||||
Record-Result "Stylua (Rojo)" $styluaRojo
|
||||
|
||||
Write-Step 2 "Auto-fix Rust Formatting"
|
||||
cargo fmt
|
||||
$rustfmt = $LASTEXITCODE
|
||||
Record-Result "Rustfmt" $rustfmt
|
||||
|
||||
Write-Step 3 "Auto-fix TypeScript Formatting (Prettier)"
|
||||
Push-Location vscode-rojo
|
||||
npx prettier --write "src/**/*.ts"
|
||||
$prettier = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Prettier" $prettier
|
||||
|
||||
Write-Step 4 "Compile TypeScript Extension (vscode-rojo)"
|
||||
Push-Location vscode-rojo
|
||||
npm run compile
|
||||
$tsBuild = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "TS Compile (vscode-rojo)" $tsBuild
|
||||
|
||||
Write-Step 5 "Lua Static Analysis (Selene)"
|
||||
selene plugin/src
|
||||
$selene = $LASTEXITCODE
|
||||
Record-Result "Selene" $selene
|
||||
|
||||
Write-Step 6 "Rust Linting (Clippy) - Auto-fix"
|
||||
cargo clippy --fix --allow-dirty --allow-staged 2>&1 | Out-Null
|
||||
Write-Host "Verifying..." -ForegroundColor Yellow
|
||||
cargo clippy --all-targets --all-features 2>&1
|
||||
$clippy = $LASTEXITCODE
|
||||
Record-Result "Clippy" $clippy
|
||||
|
||||
Write-Step 7 "Build Everything"
|
||||
cargo build --locked --all-targets --all-features
|
||||
$build = $LASTEXITCODE
|
||||
Record-Result "Build" $build
|
||||
|
||||
Write-Step 8 "Run ALL Rust Tests"
|
||||
$testOutput = cargo test --locked --all-targets --all-features 2>&1
|
||||
$rustTests = $LASTEXITCODE
|
||||
$testOutput | Write-Host
|
||||
Record-Result "Rust Tests" $rustTests
|
||||
|
||||
Write-Step 9 "Run Roblox Plugin Tests"
|
||||
.\target\debug\atlas.exe build plugin/test-place.project.json -o TestPlace.rbxl
|
||||
if ($LASTEXITCODE -eq 0) {
|
||||
run-in-roblox --script plugin/run-tests.server.lua --place TestPlace.rbxl
|
||||
$pluginTests = $LASTEXITCODE
|
||||
} else {
|
||||
Write-Host "Skipped: build failed" -ForegroundColor Yellow
|
||||
$pluginTests = 1
|
||||
}
|
||||
Record-Result "Plugin Tests" $pluginTests
|
||||
|
||||
# ─── rbx-dom ─────────────────────────────────────────────────────────────────
|
||||
|
||||
if ($RbxDom) {
|
||||
Write-Step 10 "Auto-fix Lua Formatting (Stylua) for rbx_dom_lua"
|
||||
stylua rbx-dom/rbx_dom_lua/src
|
||||
$styluaRbxDom = $LASTEXITCODE
|
||||
Record-Result "Stylua (rbx_dom_lua)" $styluaRbxDom
|
||||
|
||||
Write-Step 11 "Auto-fix Rust Formatting (rbx-dom)"
|
||||
Push-Location rbx-dom
|
||||
cargo fmt
|
||||
$rustfmtRbxDom = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Rustfmt (rbx-dom)" $rustfmtRbxDom
|
||||
|
||||
Write-Step 12 "Lua Static Analysis (Selene) for rbx_dom_lua"
|
||||
selene rbx-dom/rbx_dom_lua/src
|
||||
$seleneRbxDom = $LASTEXITCODE
|
||||
Record-Result "Selene (rbx_dom_lua)" $seleneRbxDom
|
||||
|
||||
Write-Step 13 "Rust Linting (Clippy) for rbx-dom - Auto-fix"
|
||||
Push-Location rbx-dom
|
||||
cargo clippy --fix --allow-dirty --allow-staged 2>&1 | Out-Null
|
||||
Write-Host "Verifying..." -ForegroundColor Yellow
|
||||
cargo clippy --all-targets --all-features -- -D warnings 2>&1
|
||||
$clippyRbxDom = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Clippy (rbx-dom)" $clippyRbxDom
|
||||
|
||||
Write-Step 14 "Build rbx-dom"
|
||||
Push-Location rbx-dom
|
||||
cargo build --verbose
|
||||
$buildRbxDom = $LASTEXITCODE
|
||||
cargo build --all-features --verbose
|
||||
$buildRbxDomAll = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Build (rbx-dom)" $buildRbxDom
|
||||
Record-Result "Build (rbx-dom all-features)" $buildRbxDomAll
|
||||
|
||||
Write-Step 15 "Run ALL rbx-dom Rust Tests"
|
||||
Push-Location rbx-dom
|
||||
cargo test --verbose
|
||||
$testsRbxDom = $LASTEXITCODE
|
||||
cargo test --all-features --verbose
|
||||
$testsRbxDomAll = $LASTEXITCODE
|
||||
Pop-Location
|
||||
Record-Result "Tests (rbx-dom)" $testsRbxDom
|
||||
Record-Result "Tests (rbx-dom all-features)" $testsRbxDomAll
|
||||
}
|
||||
|
||||
# ─── Report ──────────────────────────────────────────────────────────────────
|
||||
|
||||
Write-Host ""
|
||||
Write-Host "=== CI COMPLETE ===" -ForegroundColor Magenta
|
||||
Write-Host ""
|
||||
Write-Host "--- Rojo ---" -ForegroundColor White
|
||||
Write-Host ""
|
||||
Write-Host "Formatting:"
|
||||
Write-Host " - Stylua: $(if ($styluaRojo -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Rustfmt: $(if ($rustfmt -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Prettier (vscode-rojo): $(if ($prettier -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "TS Compile (vscode-rojo): $(if ($tsBuild -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Linting:"
|
||||
Write-Host " - Selene: $(if ($selene -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Clippy: $(if ($clippy -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Build: $(if ($build -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Tests:"
|
||||
Write-Host " - Rust: $(if ($rustTests -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Plugin: $(if ($pluginTests -eq 0) {'PASS'} else {'FAIL'})"
|
||||
|
||||
if ($RbxDom) {
|
||||
Write-Host ""
|
||||
Write-Host "--- rbx-dom ---" -ForegroundColor White
|
||||
Write-Host ""
|
||||
Write-Host "Formatting:"
|
||||
Write-Host " - Stylua (rbx_dom_lua): $(if ($styluaRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Rustfmt: $(if ($rustfmtRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Linting:"
|
||||
Write-Host " - Selene (rbx_dom_lua): $(if ($seleneRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Clippy: $(if ($clippyRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Build: $(if ($buildRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host "Build (all features): $(if ($buildRbxDomAll -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host ""
|
||||
Write-Host "Tests:"
|
||||
Write-Host " - Rust: $(if ($testsRbxDom -eq 0) {'PASS'} else {'FAIL'})"
|
||||
Write-Host " - Rust (all features): $(if ($testsRbxDomAll -eq 0) {'PASS'} else {'FAIL'})"
|
||||
}
|
||||
|
||||
Write-Host ""
|
||||
if ($failures.Count -eq 0) {
|
||||
Write-Host "--- Overall: PASS ---" -ForegroundColor Green
|
||||
exit 0
|
||||
} else {
|
||||
Write-Host "--- Overall: FAIL ---" -ForegroundColor Red
|
||||
Write-Host "Failed steps: $($failures -join ', ')" -ForegroundColor Red
|
||||
exit 1
|
||||
}
|
||||
181
scripts/ci.sh
Normal file
181
scripts/ci.sh
Normal file
|
|
@ -0,0 +1,181 @@
|
|||
#!/usr/bin/env bash
|
||||
# Complete CI Pipeline
|
||||
# Usage: bash scripts/ci.sh [--no-rbx-dom]
|
||||
|
||||
set -o pipefail
|
||||
|
||||
RBX_DOM=true
|
||||
for arg in "$@"; do
|
||||
case "$arg" in
|
||||
--no-rbx-dom) RBX_DOM=false ;;
|
||||
esac
|
||||
done
|
||||
|
||||
failures=()
|
||||
|
||||
step() {
|
||||
echo ""
|
||||
echo "=== Step $1: $2 ==="
|
||||
}
|
||||
|
||||
record() {
|
||||
if [ "$2" -ne 0 ]; then
|
||||
failures+=("$1")
|
||||
echo "FAIL"
|
||||
else
|
||||
echo "PASS"
|
||||
fi
|
||||
}
|
||||
|
||||
# ─── Rojo ────────────────────────────────────────────────────────────────────
|
||||
|
||||
step 1 "Auto-fix Lua Formatting (Stylua)"
|
||||
stylua plugin/src
|
||||
stylua_rojo=$?
|
||||
record "Stylua (Rojo)" $stylua_rojo
|
||||
|
||||
step 2 "Auto-fix Rust Formatting"
|
||||
cargo fmt
|
||||
rustfmt_exit=$?
|
||||
record "Rustfmt" $rustfmt_exit
|
||||
|
||||
step 3 "Auto-fix TypeScript Formatting (Prettier)"
|
||||
(cd vscode-rojo && npx prettier --write "src/**/*.ts")
|
||||
prettier_exit=$?
|
||||
record "Prettier" $prettier_exit
|
||||
|
||||
step 4 "Compile TypeScript Extension (vscode-rojo)"
|
||||
(cd vscode-rojo && npm run compile)
|
||||
ts_build=$?
|
||||
record "TS Compile (vscode-rojo)" $ts_build
|
||||
|
||||
step 5 "Lua Static Analysis (Selene)"
|
||||
selene plugin/src
|
||||
selene_exit=$?
|
||||
record "Selene" $selene_exit
|
||||
|
||||
step 6 "Rust Linting (Clippy) - Auto-fix"
|
||||
cargo clippy --fix --allow-dirty --allow-staged >/dev/null 2>&1
|
||||
echo "Verifying..."
|
||||
cargo clippy --all-targets --all-features 2>&1
|
||||
clippy_exit=$?
|
||||
record "Clippy" $clippy_exit
|
||||
|
||||
step 7 "Build Everything"
|
||||
cargo build --locked --all-targets --all-features
|
||||
build_exit=$?
|
||||
record "Build" $build_exit
|
||||
|
||||
step 8 "Run ALL Rust Tests"
|
||||
cargo test --locked --all-targets --all-features 2>&1
|
||||
rust_tests=$?
|
||||
record "Rust Tests" $rust_tests
|
||||
|
||||
step 9 "Run Roblox Plugin Tests"
|
||||
./target/debug/atlas build plugin/test-place.project.json -o TestPlace.rbxl
|
||||
if [ $? -eq 0 ]; then
|
||||
run-in-roblox --script plugin/run-tests.server.lua --place TestPlace.rbxl
|
||||
plugin_tests=$?
|
||||
else
|
||||
echo "Skipped: build failed"
|
||||
plugin_tests=1
|
||||
fi
|
||||
record "Plugin Tests" $plugin_tests
|
||||
|
||||
# ─── rbx-dom ─────────────────────────────────────────────────────────────────
|
||||
|
||||
if [ "$RBX_DOM" = true ]; then
|
||||
step 10 "Auto-fix Lua Formatting (Stylua) for rbx_dom_lua"
|
||||
(cd rbx-dom && stylua rbx_dom_lua/src)
|
||||
stylua_rbxdom=$?
|
||||
record "Stylua (rbx_dom_lua)" $stylua_rbxdom
|
||||
|
||||
step 11 "Auto-fix Rust Formatting (rbx-dom)"
|
||||
(cd rbx-dom && cargo fmt)
|
||||
rustfmt_rbxdom=$?
|
||||
record "Rustfmt (rbx-dom)" $rustfmt_rbxdom
|
||||
|
||||
step 12 "Lua Static Analysis (Selene) for rbx_dom_lua"
|
||||
(cd rbx-dom && selene rbx_dom_lua/src)
|
||||
selene_rbxdom=$?
|
||||
record "Selene (rbx_dom_lua)" $selene_rbxdom
|
||||
|
||||
step 13 "Rust Linting (Clippy) for rbx-dom - Auto-fix"
|
||||
(cd rbx-dom && cargo clippy --fix --allow-dirty --allow-staged >/dev/null 2>&1)
|
||||
echo "Verifying..."
|
||||
(cd rbx-dom && cargo clippy --all-targets --all-features -- -D warnings 2>&1)
|
||||
clippy_rbxdom=$?
|
||||
record "Clippy (rbx-dom)" $clippy_rbxdom
|
||||
|
||||
step 14 "Build rbx-dom"
|
||||
(cd rbx-dom && cargo build --verbose)
|
||||
build_rbxdom=$?
|
||||
(cd rbx-dom && cargo build --all-features --verbose)
|
||||
build_rbxdom_all=$?
|
||||
record "Build (rbx-dom)" $build_rbxdom
|
||||
record "Build (rbx-dom all-features)" $build_rbxdom_all
|
||||
|
||||
step 15 "Run ALL rbx-dom Rust Tests"
|
||||
(cd rbx-dom && cargo test --verbose)
|
||||
tests_rbxdom=$?
|
||||
(cd rbx-dom && cargo test --all-features --verbose)
|
||||
tests_rbxdom_all=$?
|
||||
record "Tests (rbx-dom)" $tests_rbxdom
|
||||
record "Tests (rbx-dom all-features)" $tests_rbxdom_all
|
||||
fi
|
||||
|
||||
# ─── Report ──────────────────────────────────────────────────────────────────
|
||||
|
||||
pass_or_fail() { [ "$1" -eq 0 ] && echo "PASS" || echo "FAIL"; }
|
||||
|
||||
echo ""
|
||||
echo "=== CI COMPLETE ==="
|
||||
echo ""
|
||||
echo "--- Rojo ---"
|
||||
echo ""
|
||||
echo "Formatting:"
|
||||
echo " - Stylua: $(pass_or_fail $stylua_rojo)"
|
||||
echo " - Rustfmt: $(pass_or_fail $rustfmt_exit)"
|
||||
echo " - Prettier (vscode-rojo): $(pass_or_fail $prettier_exit)"
|
||||
echo ""
|
||||
echo "TS Compile (vscode-rojo): $(pass_or_fail $ts_build)"
|
||||
echo ""
|
||||
echo "Linting:"
|
||||
echo " - Selene: $(pass_or_fail $selene_exit)"
|
||||
echo " - Clippy: $(pass_or_fail $clippy_exit)"
|
||||
echo ""
|
||||
echo "Build: $(pass_or_fail $build_exit)"
|
||||
echo ""
|
||||
echo "Tests:"
|
||||
echo " - Rust: $(pass_or_fail $rust_tests)"
|
||||
echo " - Plugin: $(pass_or_fail $plugin_tests)"
|
||||
|
||||
if [ "$RBX_DOM" = true ]; then
|
||||
echo ""
|
||||
echo "--- rbx-dom ---"
|
||||
echo ""
|
||||
echo "Formatting:"
|
||||
echo " - Stylua (rbx_dom_lua): $(pass_or_fail $stylua_rbxdom)"
|
||||
echo " - Rustfmt: $(pass_or_fail $rustfmt_rbxdom)"
|
||||
echo ""
|
||||
echo "Linting:"
|
||||
echo " - Selene (rbx_dom_lua): $(pass_or_fail $selene_rbxdom)"
|
||||
echo " - Clippy: $(pass_or_fail $clippy_rbxdom)"
|
||||
echo ""
|
||||
echo "Build: $(pass_or_fail $build_rbxdom)"
|
||||
echo "Build (all features): $(pass_or_fail $build_rbxdom_all)"
|
||||
echo ""
|
||||
echo "Tests:"
|
||||
echo " - Rust: $(pass_or_fail $tests_rbxdom)"
|
||||
echo " - Rust (all features): $(pass_or_fail $tests_rbxdom_all)"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
if [ ${#failures[@]} -eq 0 ]; then
|
||||
echo "--- Overall: PASS ---"
|
||||
exit 0
|
||||
else
|
||||
echo "--- Overall: FAIL ---"
|
||||
echo "Failed steps: $(IFS=', '; echo "${failures[*]}")"
|
||||
exit 1
|
||||
fi
|
||||
|
|
@ -11,11 +11,11 @@ use std::{
|
|||
|
||||
use crate::{
|
||||
message_queue::MessageQueue,
|
||||
path_encoding::encode_path_name,
|
||||
snapshot::{
|
||||
apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree,
|
||||
},
|
||||
snapshot_middleware::{snapshot_from_vfs, snapshot_project_node},
|
||||
syncback::{deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix},
|
||||
};
|
||||
|
||||
/// Processes file change events, updates the DOM, and sends those updates
|
||||
|
|
@ -226,6 +226,101 @@ impl JobThreadContext {
|
|||
}
|
||||
}
|
||||
|
||||
/// Suppress the next Remove VFS event for the given path.
|
||||
fn suppress_path_remove(&self, path: &Path) {
|
||||
let mut suppressed = self.suppressed_paths.lock().unwrap();
|
||||
let key = Self::suppression_key(path);
|
||||
suppressed.entry(key).or_insert((0, 0)).0 += 1;
|
||||
}
|
||||
|
||||
/// Upsert the `name` field in a `.meta.json5` file, suppressing filesystem
|
||||
/// events to avoid feedback loops.
|
||||
fn upsert_meta_name_field(&self, meta_path: &Path, real_name: &str) {
|
||||
self.suppress_path(meta_path);
|
||||
if let Err(err) = crate::syncback::meta::upsert_meta_name(meta_path, real_name) {
|
||||
self.unsuppress_path(meta_path);
|
||||
log::error!(
|
||||
"Failed to upsert name in meta file {}: {}",
|
||||
meta_path.display(),
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Upsert the `name` field inside a `.model.json5` / `.model.json` file,
|
||||
/// suppressing filesystem events.
|
||||
fn upsert_model_name_field(&self, model_path: &Path, real_name: &str) {
|
||||
self.suppress_path(model_path);
|
||||
if let Err(err) = crate::syncback::meta::upsert_model_name(model_path, real_name) {
|
||||
self.unsuppress_path(model_path);
|
||||
log::error!(
|
||||
"Failed to upsert name in model file {}: {}",
|
||||
model_path.display(),
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Remove the `name` field from a `.model.json5` / `.model.json` file,
|
||||
/// suppressing filesystem events.
|
||||
fn remove_model_name_field(&self, model_path: &Path) {
|
||||
use crate::syncback::meta::RemoveNameOutcome;
|
||||
self.suppress_path(model_path);
|
||||
match crate::syncback::meta::remove_model_name(model_path) {
|
||||
Ok(RemoveNameOutcome::NoOp) => {
|
||||
self.unsuppress_path(model_path);
|
||||
}
|
||||
Ok(RemoveNameOutcome::FieldRemoved) => {
|
||||
// File was rewritten — suppress_path already covers it.
|
||||
}
|
||||
Ok(RemoveNameOutcome::FileDeleted) => {
|
||||
// Model files shouldn't be deleted (they have className etc),
|
||||
// but handle for completeness.
|
||||
self.unsuppress_path(model_path);
|
||||
self.suppress_path_remove(model_path);
|
||||
}
|
||||
Err(err) => {
|
||||
self.unsuppress_path(model_path);
|
||||
log::error!(
|
||||
"Failed to remove name from model file {}: {}",
|
||||
model_path.display(),
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Remove the `name` field from a `.meta.json5` file, suppressing filesystem
|
||||
/// events. If the file becomes empty after removal, deletes it entirely.
|
||||
fn remove_meta_name_field(&self, meta_path: &Path) {
|
||||
use crate::syncback::meta::RemoveNameOutcome;
|
||||
// Suppress for the write/delete that may follow
|
||||
self.suppress_path(meta_path);
|
||||
match crate::syncback::meta::remove_meta_name(meta_path) {
|
||||
Ok(RemoveNameOutcome::NoOp) => {
|
||||
self.unsuppress_path(meta_path);
|
||||
}
|
||||
Ok(RemoveNameOutcome::FileDeleted) => {
|
||||
// File was deleted, not rewritten. Swap: undo the
|
||||
// pre-emptive Write suppression and add a Remove
|
||||
// suppression instead so the counts are (1, 0).
|
||||
self.unsuppress_path(meta_path);
|
||||
self.suppress_path_remove(meta_path);
|
||||
}
|
||||
Ok(RemoveNameOutcome::FieldRemoved) => {
|
||||
// File was rewritten — suppress_path already covers it.
|
||||
}
|
||||
Err(err) => {
|
||||
self.unsuppress_path(meta_path);
|
||||
log::error!(
|
||||
"Failed to remove name from meta file {}: {}",
|
||||
meta_path.display(),
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Computes and applies patches to the DOM for a given file path.
|
||||
///
|
||||
/// This function finds the nearest ancestor to the given path that has associated instances
|
||||
|
|
@ -561,47 +656,27 @@ impl JobThreadContext {
|
|||
let applied_patch = {
|
||||
let mut tree = self.tree.lock().unwrap();
|
||||
|
||||
// NOTE: We do NOT delete files from disk here. The API handler
|
||||
// (handle_api_write → remove_instance_at_path) already deleted
|
||||
// the files before sending this PatchSet. Re-deleting would
|
||||
// destroy any new file created at the same path — e.g., when an
|
||||
// instance is renamed from "joe_test" to "joe/test", the new slug
|
||||
// is also "joe_test", so the API creates a new file at the same
|
||||
// path that the removal just deleted. If we deleted again here,
|
||||
// the new file would be destroyed.
|
||||
//
|
||||
// The PatchSet removal only needs to update the DOM tree (handled
|
||||
// by apply_patch_set below).
|
||||
for &id in &patch_set.removed_instances {
|
||||
if let Some(instance) = tree.get_instance(id) {
|
||||
if let Some(instigating_source) = &instance.metadata().instigating_source {
|
||||
match instigating_source {
|
||||
InstigatingSource::Path(path) => {
|
||||
// Guard: file may already be deleted by the API's
|
||||
// syncback_removed_instance before this PatchSet arrives.
|
||||
if path.exists() {
|
||||
if path.is_dir() {
|
||||
log::info!(
|
||||
"Two-way sync: Removing directory {}",
|
||||
path.display()
|
||||
);
|
||||
if let Err(err) = fs::remove_dir_all(path) {
|
||||
log::error!(
|
||||
"Failed to remove directory {:?} for instance {:?}: {}",
|
||||
path,
|
||||
id,
|
||||
err
|
||||
);
|
||||
}
|
||||
} else {
|
||||
log::info!(
|
||||
"Two-way sync: Removing file {}",
|
||||
path.display()
|
||||
);
|
||||
if let Err(err) = fs::remove_file(path) {
|
||||
log::error!(
|
||||
"Failed to remove file {:?} for instance {:?}: {}",
|
||||
path,
|
||||
id,
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
log::info!(
|
||||
"Two-way sync: File already removed: {}",
|
||||
path.display()
|
||||
);
|
||||
}
|
||||
log::info!(
|
||||
"Two-way sync: Removing instance {:?} from tree (path: {})",
|
||||
id,
|
||||
path.display()
|
||||
);
|
||||
}
|
||||
InstigatingSource::ProjectNode { .. } => {
|
||||
log::warn!(
|
||||
|
|
@ -653,17 +728,27 @@ impl JobThreadContext {
|
|||
if is_init_file {
|
||||
let dir_path = path.parent().unwrap();
|
||||
if let Some(grandparent) = dir_path.parent() {
|
||||
// Use the encoded dir name from the filesystem
|
||||
// for the old meta lookup, and encode the new
|
||||
// name for the new path. Using instance.name()
|
||||
// (decoded) would fail for encoded names.
|
||||
let dir_name = dir_path
|
||||
.file_name()
|
||||
.and_then(|f| f.to_str())
|
||||
.unwrap_or("");
|
||||
let encoded_new_name = encode_path_name(new_name);
|
||||
// Slugify the new name for filesystem safety
|
||||
let slugified_new_name =
|
||||
if name_needs_slugify(new_name) {
|
||||
slugify_name(new_name)
|
||||
} else {
|
||||
new_name.to_string()
|
||||
};
|
||||
// Directory renames: fs::rename fails safely
|
||||
// on all platforms if the target is a non-empty
|
||||
// directory, so no dedup is needed here (unlike
|
||||
// the file rename path below, where Unix
|
||||
// fs::rename silently overwrites).
|
||||
let new_dir_path =
|
||||
grandparent.join(&encoded_new_name);
|
||||
grandparent.join(&slugified_new_name);
|
||||
|
||||
// Track the effective directory path after any rename
|
||||
let effective_dir_path;
|
||||
|
||||
if new_dir_path != dir_path {
|
||||
log::info!(
|
||||
|
|
@ -684,6 +769,7 @@ impl JobThreadContext {
|
|||
new_dir_path,
|
||||
err
|
||||
);
|
||||
effective_dir_path = dir_path.to_path_buf();
|
||||
} else {
|
||||
// The init file moved with the directory.
|
||||
overridden_source_path =
|
||||
|
|
@ -696,7 +782,7 @@ impl JobThreadContext {
|
|||
let new_meta =
|
||||
grandparent.join(format!(
|
||||
"{}.meta.json5",
|
||||
encoded_new_name
|
||||
slugified_new_name
|
||||
));
|
||||
self.suppress_path_any(&old_meta);
|
||||
self.suppress_path(&new_meta);
|
||||
|
|
@ -707,14 +793,31 @@ impl JobThreadContext {
|
|||
self.unsuppress_path(&new_meta);
|
||||
}
|
||||
}
|
||||
effective_dir_path = new_dir_path.clone();
|
||||
}
|
||||
} else {
|
||||
effective_dir_path = dir_path.to_path_buf();
|
||||
}
|
||||
|
||||
// Always update the meta name field when the
|
||||
// instance name changed, even if the directory
|
||||
// path didn't change (e.g. "Foo/Bar" → "Foo|Bar"
|
||||
// both slugify to "Foo_Bar").
|
||||
let init_meta =
|
||||
effective_dir_path.join("init.meta.json5");
|
||||
if slugified_new_name != *new_name {
|
||||
self.upsert_meta_name_field(
|
||||
&init_meta, new_name,
|
||||
);
|
||||
} else {
|
||||
self.remove_meta_name_field(&init_meta);
|
||||
}
|
||||
}
|
||||
} else if let Some(parent) = path.parent() {
|
||||
// Derive the suffix from the existing filename
|
||||
// rather than using replacen with the decoded
|
||||
// instance name, which fails for encoded names
|
||||
// (e.g., What%QUESTION%.server.luau).
|
||||
// rather than using replacen with the instance
|
||||
// name, which may differ from the filesystem
|
||||
// name (e.g., slugified names).
|
||||
let extension = path
|
||||
.extension()
|
||||
.and_then(|e| e.to_str())
|
||||
|
|
@ -725,7 +828,7 @@ impl JobThreadContext {
|
|||
.unwrap_or("");
|
||||
let known_suffixes = [
|
||||
".server", ".client", ".plugin", ".local",
|
||||
".legacy",
|
||||
".legacy", ".model",
|
||||
];
|
||||
let script_suffix = known_suffixes
|
||||
.iter()
|
||||
|
|
@ -738,17 +841,79 @@ impl JobThreadContext {
|
|||
&stem[..stem.len() - script_suffix.len()]
|
||||
};
|
||||
|
||||
let encoded_new_name = encode_path_name(new_name);
|
||||
let slugified_new_name = if name_needs_slugify(new_name)
|
||||
{
|
||||
slugify_name(new_name)
|
||||
} else {
|
||||
new_name.to_string()
|
||||
};
|
||||
|
||||
// Guard against rename collision: if the target
|
||||
// path already exists and isn't our own file,
|
||||
// deduplicate the name against siblings.
|
||||
let deduped_new_name;
|
||||
{
|
||||
let candidate_file = if extension.is_empty() {
|
||||
format!(
|
||||
"{}{}",
|
||||
slugified_new_name, script_suffix
|
||||
)
|
||||
} else {
|
||||
format!(
|
||||
"{}{}.{}",
|
||||
slugified_new_name,
|
||||
script_suffix,
|
||||
extension
|
||||
)
|
||||
};
|
||||
let candidate = parent.join(&candidate_file);
|
||||
if candidate != *path && candidate.exists() {
|
||||
let mut taken: std::collections::HashSet<
|
||||
String,
|
||||
> = std::collections::HashSet::new();
|
||||
if let Ok(entries) = fs::read_dir(parent) {
|
||||
for entry in entries.flatten() {
|
||||
let ep = entry.path();
|
||||
let slug = if ep.is_dir() {
|
||||
ep.file_name()
|
||||
.and_then(|f| f.to_str())
|
||||
.unwrap_or("")
|
||||
.to_lowercase()
|
||||
} else {
|
||||
let s = ep
|
||||
.file_stem()
|
||||
.and_then(|f| f.to_str())
|
||||
.unwrap_or("");
|
||||
strip_script_suffix(s)
|
||||
.to_lowercase()
|
||||
};
|
||||
taken.insert(slug);
|
||||
}
|
||||
}
|
||||
// Free the slot we're vacating
|
||||
taken.remove(&old_base.to_lowercase());
|
||||
deduped_new_name = deduplicate_name(
|
||||
&slugified_new_name,
|
||||
&taken,
|
||||
);
|
||||
} else {
|
||||
deduped_new_name = slugified_new_name.clone();
|
||||
}
|
||||
}
|
||||
|
||||
let new_file_name = if extension.is_empty() {
|
||||
format!("{}{}", encoded_new_name, script_suffix)
|
||||
format!("{}{}", deduped_new_name, script_suffix)
|
||||
} else {
|
||||
format!(
|
||||
"{}{}.{}",
|
||||
encoded_new_name, script_suffix, extension
|
||||
deduped_new_name, script_suffix, extension
|
||||
)
|
||||
};
|
||||
let new_path = parent.join(&new_file_name);
|
||||
|
||||
// Track the effective meta base name after any rename
|
||||
let effective_meta_base: &str;
|
||||
|
||||
if new_path != *path {
|
||||
log::info!(
|
||||
"Two-way sync: Renaming {} -> {}",
|
||||
|
|
@ -766,15 +931,16 @@ impl JobThreadContext {
|
|||
new_path,
|
||||
err
|
||||
);
|
||||
effective_meta_base = old_base;
|
||||
} else {
|
||||
overridden_source_path = Some(new_path.clone());
|
||||
let old_meta = parent
|
||||
.join(format!("{}.meta.json5", old_base));
|
||||
let new_meta = parent.join(format!(
|
||||
"{}.meta.json5",
|
||||
deduped_new_name
|
||||
));
|
||||
if old_meta.exists() {
|
||||
let new_meta = parent.join(format!(
|
||||
"{}.meta.json5",
|
||||
encoded_new_name
|
||||
));
|
||||
self.suppress_path_any(&old_meta);
|
||||
self.suppress_path(&new_meta);
|
||||
if fs::rename(&old_meta, &new_meta).is_err()
|
||||
|
|
@ -783,6 +949,44 @@ impl JobThreadContext {
|
|||
self.unsuppress_path(&new_meta);
|
||||
}
|
||||
}
|
||||
effective_meta_base = &deduped_new_name;
|
||||
}
|
||||
} else {
|
||||
effective_meta_base = old_base;
|
||||
}
|
||||
|
||||
// Always update the name field when the
|
||||
// instance name changed, even if the path didn't
|
||||
// change (e.g. "Foo/Bar" → "Foo|Bar" both slugify
|
||||
// to "Foo_Bar"). Use deduped_new_name because
|
||||
// dedup may have appended ~N.
|
||||
//
|
||||
// For .model.json5/.model.json files, the name
|
||||
// field lives INSIDE the model file, not in
|
||||
// adjacent .meta.json5.
|
||||
if script_suffix == ".model" {
|
||||
let model_file = overridden_source_path
|
||||
.as_deref()
|
||||
.unwrap_or(path.as_path());
|
||||
if deduped_new_name != *new_name {
|
||||
self.upsert_model_name_field(
|
||||
model_file, new_name,
|
||||
);
|
||||
} else {
|
||||
self.remove_model_name_field(model_file);
|
||||
}
|
||||
} else {
|
||||
let current_meta = parent.join(format!(
|
||||
"{}.meta.json5",
|
||||
effective_meta_base
|
||||
));
|
||||
if deduped_new_name != *new_name {
|
||||
self.upsert_meta_name_field(
|
||||
¤t_meta,
|
||||
new_name,
|
||||
);
|
||||
} else {
|
||||
self.remove_meta_name_field(¤t_meta);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -861,10 +1065,9 @@ impl JobThreadContext {
|
|||
format!("init{}.luau", new_suffix)
|
||||
}
|
||||
} else {
|
||||
// Derive the encoded base name from the
|
||||
// filesystem path, not instance.name()
|
||||
// (which is decoded). This preserves
|
||||
// encoding like What%QUESTION% on disk.
|
||||
// Derive the base name from the filesystem
|
||||
// path, not instance.name() (which may
|
||||
// differ from the slugified filename).
|
||||
let stem = actual_file
|
||||
.file_stem()
|
||||
.and_then(|s| s.to_str())
|
||||
|
|
|
|||
|
|
@ -13,7 +13,6 @@ mod json;
|
|||
mod lua_ast;
|
||||
mod message_queue;
|
||||
mod multimap;
|
||||
pub mod path_encoding;
|
||||
mod path_serializer;
|
||||
mod project;
|
||||
mod resolution;
|
||||
|
|
|
|||
|
|
@ -1,342 +0,0 @@
|
|||
//! Encoding and decoding utilities for special filename characters.
|
||||
//!
|
||||
//! This module encodes characters that are problematic in file names:
|
||||
//! - Windows-invalid characters: < > : " / \ | ? *
|
||||
//! - Periods (.) to prevent conflicts with file extension parsing
|
||||
//! - Leading/trailing spaces
|
||||
//! - Literal percent signs (%) to allow round-tripping
|
||||
//!
|
||||
//! Characters are encoded using a %NAME% format (e.g., `<` becomes `%LT%`,
|
||||
//! `.` becomes `%DOT%`). Literal `%` is escaped as `%%` (like printf).
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::OnceLock;
|
||||
|
||||
/// Mapping of special characters to their encoded representations.
|
||||
const CHAR_ENCODINGS: &[(&str, &str)] = &[
|
||||
(".", "%DOT%"),
|
||||
("<", "%LT%"),
|
||||
(">", "%GT%"),
|
||||
(":", "%COLON%"),
|
||||
("\"", "%QUOTE%"),
|
||||
("/", "%SLASH%"),
|
||||
("\\", "%BACKSLASH%"),
|
||||
("|", "%PIPE%"),
|
||||
("?", "%QUESTION%"),
|
||||
("*", "%STAR%"),
|
||||
];
|
||||
|
||||
/// Returns a map from encoded pattern (e.g., "DOT") to decoded char (e.g., ".").
|
||||
fn get_decode_map() -> &'static HashMap<&'static str, &'static str> {
|
||||
static DECODE_MAP: OnceLock<HashMap<&'static str, &'static str>> = OnceLock::new();
|
||||
DECODE_MAP.get_or_init(|| {
|
||||
let mut map = HashMap::new();
|
||||
for (char, encoded) in CHAR_ENCODINGS {
|
||||
// Strip the leading and trailing % from the pattern
|
||||
let pattern = &encoded[1..encoded.len() - 1];
|
||||
map.insert(pattern, *char);
|
||||
}
|
||||
map
|
||||
})
|
||||
}
|
||||
|
||||
const SPACE_ENCODING: &str = "%SPACE%";
|
||||
|
||||
/// Encodes special characters in a file name.
|
||||
///
|
||||
/// This replaces characters like `.`, `<`, `>`, `:`, `"`, `/`, `\`, `|`, `?`, `*`
|
||||
/// with their encoded representations like `%DOT%`, `%LT%`, `%GT%`, etc.
|
||||
/// Literal `%` is escaped as `%%`. Leading and trailing spaces are encoded as `%SPACE%`.
|
||||
pub fn encode_path_name(name: &str) -> String {
|
||||
let trimmed = name.trim_matches(' ');
|
||||
|
||||
// Handle the case where the string is entirely spaces (or empty)
|
||||
if trimmed.is_empty() {
|
||||
return SPACE_ENCODING.repeat(name.len());
|
||||
}
|
||||
|
||||
// Count leading and trailing spaces
|
||||
let leading_spaces = name.len() - name.trim_start_matches(' ').len();
|
||||
let trailing_spaces = name.len() - name.trim_end_matches(' ').len();
|
||||
|
||||
// Get the middle part (without leading/trailing spaces)
|
||||
let middle = &name[leading_spaces..name.len() - trailing_spaces];
|
||||
|
||||
// First: escape % as %% (before adding new % signs from encodings)
|
||||
let mut encoded_middle = middle.replace('%', "%%");
|
||||
|
||||
// Then: encode special characters
|
||||
for (char, encoded) in CHAR_ENCODINGS {
|
||||
encoded_middle = encoded_middle.replace(char, encoded);
|
||||
}
|
||||
|
||||
// Build final result with encoded spaces
|
||||
let encoded_prefix = SPACE_ENCODING.repeat(leading_spaces);
|
||||
let encoded_suffix = SPACE_ENCODING.repeat(trailing_spaces);
|
||||
|
||||
format!("{}{}{}", encoded_prefix, encoded_middle, encoded_suffix)
|
||||
}
|
||||
|
||||
/// Decodes encoded special characters back to their original form.
|
||||
///
|
||||
/// This replaces encoded representations like `%DOT%`, `%LT%`, `%GT%`, etc.
|
||||
/// back to their original characters `.`, `<`, `>`, etc.
|
||||
/// `%%` is decoded to literal `%`. `%SPACE%` at the start/end is decoded back to spaces.
|
||||
///
|
||||
/// Uses a proper left-to-right parser to correctly handle `%%` escaping.
|
||||
pub fn decode_path_name(name: &str) -> String {
|
||||
let mut result = name.to_string();
|
||||
|
||||
// Count and strip leading %SPACE%
|
||||
let mut leading_spaces = 0;
|
||||
while result.starts_with(SPACE_ENCODING) {
|
||||
leading_spaces += 1;
|
||||
result = result[SPACE_ENCODING.len()..].to_string();
|
||||
}
|
||||
|
||||
// Count and strip trailing %SPACE%
|
||||
let mut trailing_spaces = 0;
|
||||
while result.ends_with(SPACE_ENCODING) {
|
||||
trailing_spaces += 1;
|
||||
result = result[..result.len() - SPACE_ENCODING.len()].to_string();
|
||||
}
|
||||
|
||||
// Parse and decode the middle part
|
||||
let decoded_middle = decode_patterns(&result);
|
||||
|
||||
// Rebuild with actual spaces
|
||||
let prefix = " ".repeat(leading_spaces);
|
||||
let suffix = " ".repeat(trailing_spaces);
|
||||
format!("{}{}{}", prefix, decoded_middle, suffix)
|
||||
}
|
||||
|
||||
/// Parses an encoded string left-to-right, properly handling `%%` escapes and `%NAME%` patterns.
|
||||
fn decode_patterns(input: &str) -> String {
|
||||
let decode_map = get_decode_map();
|
||||
let chars: Vec<char> = input.chars().collect();
|
||||
let mut output = String::with_capacity(input.len());
|
||||
let mut i = 0;
|
||||
|
||||
while i < chars.len() {
|
||||
if chars[i] == '%' {
|
||||
// Check for %% (escaped percent)
|
||||
if i + 1 < chars.len() && chars[i + 1] == '%' {
|
||||
output.push('%');
|
||||
i += 2;
|
||||
continue;
|
||||
}
|
||||
|
||||
// Try to match a %NAME% pattern
|
||||
if let Some((decoded, consumed)) = try_decode_pattern(&chars, i, decode_map) {
|
||||
output.push_str(&decoded);
|
||||
i += consumed;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
// Regular character, just copy it
|
||||
output.push(chars[i]);
|
||||
i += 1;
|
||||
}
|
||||
|
||||
output
|
||||
}
|
||||
|
||||
/// Tries to decode a %NAME% pattern starting at position `start`.
|
||||
/// Returns Some((decoded_string, chars_consumed)) if successful, None otherwise.
|
||||
fn try_decode_pattern(
|
||||
chars: &[char],
|
||||
start: usize,
|
||||
decode_map: &HashMap<&str, &str>,
|
||||
) -> Option<(String, usize)> {
|
||||
// Must start with %
|
||||
if chars.get(start) != Some(&'%') {
|
||||
return None;
|
||||
}
|
||||
|
||||
// Find the closing %
|
||||
let mut end = start + 1;
|
||||
while end < chars.len() && chars[end] != '%' {
|
||||
// Pattern names are uppercase letters only
|
||||
if !chars[end].is_ascii_uppercase() {
|
||||
return None;
|
||||
}
|
||||
end += 1;
|
||||
}
|
||||
|
||||
// Must have found a closing %
|
||||
if end >= chars.len() || chars[end] != '%' {
|
||||
return None;
|
||||
}
|
||||
|
||||
// Extract the pattern name (without the % delimiters)
|
||||
let pattern_name: String = chars[start + 1..end].iter().collect();
|
||||
|
||||
// Look up the pattern
|
||||
if let Some(&decoded) = decode_map.get(pattern_name.as_str()) {
|
||||
Some((decoded.to_string(), end - start + 1))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn test_encode_all_chars() {
|
||||
let input = r#".<>:"/\|?*"#;
|
||||
let expected = "%DOT%%LT%%GT%%COLON%%QUOTE%%SLASH%%BACKSLASH%%PIPE%%QUESTION%%STAR%";
|
||||
assert_eq!(encode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_decode_all_chars() {
|
||||
let input = "%DOT%%LT%%GT%%COLON%%QUOTE%%SLASH%%BACKSLASH%%PIPE%%QUESTION%%STAR%";
|
||||
let expected = r#".<>:"/\|?*"#;
|
||||
assert_eq!(decode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_roundtrip() {
|
||||
let original = "Test<File>With:Special\"Chars/And\\More|Stuff?Here*End";
|
||||
let encoded = encode_path_name(original);
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_no_special_chars() {
|
||||
let input = "NormalFileName";
|
||||
assert_eq!(encode_path_name(input), input);
|
||||
assert_eq!(decode_path_name(input), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_leading_spaces() {
|
||||
let input = " LeadingSpaces";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "%SPACE%%SPACE%LeadingSpaces");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_trailing_spaces() {
|
||||
let input = "TrailingSpaces ";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "TrailingSpaces%SPACE%%SPACE%");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_both_spaces() {
|
||||
let input = " Both Spaces ";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "%SPACE%Both Spaces%SPACE%");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_middle_spaces_unchanged() {
|
||||
let input = "Middle Spaces Here";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "Middle Spaces Here");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_roundtrip_with_spaces() {
|
||||
let original = " <Test> ";
|
||||
let encoded = encode_path_name(original);
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_encode_period() {
|
||||
let input = "My.Script";
|
||||
let expected = "My%DOT%Script";
|
||||
assert_eq!(encode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_decode_period() {
|
||||
let input = "My%DOT%Script";
|
||||
let expected = "My.Script";
|
||||
assert_eq!(decode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_roundtrip_with_period() {
|
||||
let original = "My.Module.Name";
|
||||
let encoded = encode_path_name(original);
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_encode_percent() {
|
||||
let input = "My%Thing";
|
||||
let expected = "My%%Thing";
|
||||
assert_eq!(encode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_decode_percent() {
|
||||
let input = "My%%Thing";
|
||||
let expected = "My%Thing";
|
||||
assert_eq!(decode_path_name(input), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_roundtrip_with_percent() {
|
||||
let original = "My%Thing";
|
||||
let encoded = encode_path_name(original);
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_percent_encoding_pattern_roundtrip() {
|
||||
// This tests that a name containing what looks like an encoding pattern
|
||||
// round-trips correctly and doesn't get incorrectly decoded
|
||||
let original = "My%DOT%Thing";
|
||||
let encoded = encode_path_name(original);
|
||||
assert_eq!(encoded, "My%%DOT%%Thing");
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_combined_period_and_percent() {
|
||||
let original = "My.%Thing.Other";
|
||||
let encoded = encode_path_name(original);
|
||||
let decoded = decode_path_name(&encoded);
|
||||
assert_eq!(decoded, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_single_space() {
|
||||
let input = " ";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "%SPACE%");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_multiple_spaces_only() {
|
||||
let input = " ";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "%SPACE%%SPACE%%SPACE%");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_empty_string() {
|
||||
let input = "";
|
||||
let encoded = encode_path_name(input);
|
||||
assert_eq!(encoded, "");
|
||||
assert_eq!(decode_path_name(&encoded), input);
|
||||
}
|
||||
}
|
||||
|
|
@ -114,14 +114,7 @@ impl ServeSession {
|
|||
|
||||
let root_id = tree.get_root_id();
|
||||
|
||||
let decode_windows_invalid_chars = root_project
|
||||
.syncback_rules
|
||||
.as_ref()
|
||||
.map(|rules| rules.encode_windows_invalid_chars())
|
||||
.unwrap_or(true);
|
||||
|
||||
let mut instance_context = InstanceContext::new();
|
||||
instance_context.set_decode_windows_invalid_chars(decode_windows_invalid_chars);
|
||||
let instance_context = InstanceContext::new();
|
||||
|
||||
log::trace!("Generating snapshot of instances from VFS");
|
||||
let snapshot = snapshot_from_vfs(&instance_context, &vfs, start_path)?;
|
||||
|
|
|
|||
|
|
@ -66,6 +66,12 @@ pub struct InstanceMetadata {
|
|||
/// A schema provided via a JSON file, if one exists. Will be `None` for
|
||||
/// all non-JSON middleware.
|
||||
pub schema: Option<String>,
|
||||
|
||||
/// A custom name specified via meta.json or model.json files. If present,
|
||||
/// this name will be used for the instance while the filesystem name will
|
||||
/// be slugified to remove illegal characters.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub specified_name: Option<String>,
|
||||
}
|
||||
|
||||
impl InstanceMetadata {
|
||||
|
|
@ -78,6 +84,7 @@ impl InstanceMetadata {
|
|||
specified_id: None,
|
||||
middleware: None,
|
||||
schema: None,
|
||||
specified_name: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -126,6 +133,13 @@ impl InstanceMetadata {
|
|||
pub fn schema(self, schema: Option<String>) -> Self {
|
||||
Self { schema, ..self }
|
||||
}
|
||||
|
||||
pub fn specified_name(self, specified_name: Option<String>) -> Self {
|
||||
Self {
|
||||
specified_name,
|
||||
..self
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Default for InstanceMetadata {
|
||||
|
|
@ -140,16 +154,6 @@ pub struct InstanceContext {
|
|||
pub path_ignore_rules: Arc<Vec<PathIgnoreRule>>,
|
||||
#[serde(skip_serializing_if = "Vec::is_empty")]
|
||||
pub sync_rules: Vec<SyncRule>,
|
||||
/// Whether to decode special characters in file names.
|
||||
/// Characters like `%DOT%`, `%LT%`, `%GT%`, `%COLON%`, etc. will be decoded
|
||||
/// back to `.`, `<`, `>`, `:`, etc. in instance names sent to the plugin.
|
||||
/// Defaults to true.
|
||||
#[serde(default = "decode_windows_invalid_chars_default")]
|
||||
pub decode_windows_invalid_chars: bool,
|
||||
}
|
||||
|
||||
fn decode_windows_invalid_chars_default() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
impl InstanceContext {
|
||||
|
|
@ -157,14 +161,9 @@ impl InstanceContext {
|
|||
Self {
|
||||
path_ignore_rules: Arc::new(Vec::new()),
|
||||
sync_rules: Vec::new(),
|
||||
decode_windows_invalid_chars: true,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn set_decode_windows_invalid_chars(&mut self, decode: bool) {
|
||||
self.decode_windows_invalid_chars = decode;
|
||||
}
|
||||
|
||||
/// Extend the list of ignore rules in the context with the given new rules.
|
||||
pub fn add_path_ignore_rules<I>(&mut self, new_rules: I)
|
||||
where
|
||||
|
|
|
|||
|
|
@ -11,8 +11,7 @@ properties:
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ properties: {}
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -11,8 +11,7 @@ properties:
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ properties: {}
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ added_instances:
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,9 +10,8 @@ use rbx_dom_weak::{types::Variant, ustr};
|
|||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use crate::{
|
||||
path_encoding::encode_path_name,
|
||||
snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot},
|
||||
syncback::{FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
syncback::{name_needs_slugify, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
};
|
||||
|
||||
use super::{
|
||||
|
|
@ -78,10 +77,16 @@ pub fn snapshot_csv_init(
|
|||
|
||||
let mut init_snapshot = snapshot_csv(context, vfs, init_path, &dir_snapshot.name)?.unwrap();
|
||||
|
||||
// Preserve the init script's instigating_source (the actual file path)
|
||||
// before copying the directory's metadata (which has the folder path)
|
||||
let script_instigating_source = init_snapshot.metadata.instigating_source.take();
|
||||
|
||||
init_snapshot.children = dir_snapshot.children;
|
||||
init_snapshot.metadata = dir_snapshot.metadata;
|
||||
// The directory snapshot middleware includes all possible init paths
|
||||
// so we don't need to add it here.
|
||||
|
||||
// Restore the init script's instigating_source so two-way sync writes
|
||||
// to the actual file (e.g., init.csv) instead of the directory
|
||||
init_snapshot.metadata.instigating_source = script_instigating_source;
|
||||
|
||||
DirectoryMetadata::read_and_apply_all(vfs, folder_path, &mut init_snapshot)?;
|
||||
|
||||
|
|
@ -110,10 +115,20 @@ pub fn syncback_csv<'sync>(
|
|||
|
||||
if !meta.is_empty() {
|
||||
let parent = snapshot.path.parent_err()?;
|
||||
let meta_name = if snapshot.encode_windows_invalid_chars() {
|
||||
encode_path_name(&new_inst.name)
|
||||
let meta_name = snapshot
|
||||
.path
|
||||
.file_stem()
|
||||
.and_then(|s| s.to_str())
|
||||
.unwrap_or("");
|
||||
let meta_name = if meta_name.is_empty() {
|
||||
let instance_name = &new_inst.name;
|
||||
if name_needs_slugify(instance_name) {
|
||||
slugify_name(instance_name)
|
||||
} else {
|
||||
instance_name.clone()
|
||||
}
|
||||
} else {
|
||||
new_inst.name.clone()
|
||||
meta_name.to_string()
|
||||
};
|
||||
fs_snapshot.add_file(
|
||||
parent.join(format!("{}.meta.json5", meta_name)),
|
||||
|
|
|
|||
|
|
@ -8,7 +8,11 @@ use memofs::{DirEntry, Vfs};
|
|||
|
||||
use crate::{
|
||||
snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource},
|
||||
syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
snapshot_middleware::Middleware,
|
||||
syncback::{
|
||||
hash_instance, name_needs_slugify, slugify_name, strip_middleware_extension, FsSnapshot,
|
||||
SyncbackReturn, SyncbackSnapshot,
|
||||
},
|
||||
};
|
||||
|
||||
use super::{meta_file::DirectoryMetadata, snapshot_from_vfs};
|
||||
|
|
@ -138,6 +142,12 @@ pub fn syncback_dir_no_meta<'sync>(
|
|||
let mut children = Vec::new();
|
||||
let mut removed_children = Vec::new();
|
||||
|
||||
// taken_names tracks claimed bare slugs (without extensions) for dedup.
|
||||
// Pre-seeded from old tree children's slugified instance names so that
|
||||
// new-only children correctly dedup against existing siblings, regardless
|
||||
// of iteration order (see plan: fix_stem-level_dedup §2).
|
||||
let mut taken_names: HashSet<String> = HashSet::new();
|
||||
|
||||
// Detect duplicate child names (case-insensitive for file system safety).
|
||||
// We skip duplicates instead of failing, tracking them in stats.
|
||||
let mut child_name_counts: HashMap<String, usize> = HashMap::new();
|
||||
|
|
@ -177,7 +187,49 @@ pub fn syncback_dir_no_meta<'sync>(
|
|||
old_child_map.insert(inst.name(), inst);
|
||||
}
|
||||
|
||||
for new_child_ref in new_inst.children() {
|
||||
// Pre-seed taken_names from old children's actual filesystem dedup keys
|
||||
// so that new-only children correctly dedup against existing siblings.
|
||||
// We derive keys from relevant_paths (the real filename on disk) rather
|
||||
// than slugifying instance names, because an old instance may already
|
||||
// have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare
|
||||
// slug wouldn't capture.
|
||||
// Only in incremental mode: in clean mode, old_ref is forced to None
|
||||
// for all children, so every child is treated as new and pre-seeding
|
||||
// would cause false collisions (spurious ~1 suffixes).
|
||||
if snapshot.data.is_incremental() {
|
||||
for inst in old_child_map.values() {
|
||||
if let Some(path) = inst.metadata().relevant_paths.first() {
|
||||
if let Some(filename) = path.file_name().and_then(|f| f.to_str()) {
|
||||
let middleware = inst.metadata().middleware.unwrap_or(Middleware::Dir);
|
||||
let dedup_key =
|
||||
strip_middleware_extension(filename, middleware).to_lowercase();
|
||||
taken_names.insert(dedup_key);
|
||||
}
|
||||
} else {
|
||||
// Fallback (shouldn't happen for old instances with disk presence)
|
||||
let name = inst.name();
|
||||
let slug = if name_needs_slugify(name) {
|
||||
slugify_name(name).to_lowercase()
|
||||
} else {
|
||||
name.to_lowercase()
|
||||
};
|
||||
taken_names.insert(slug);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Sort children alphabetically for deterministic dedup ordering.
|
||||
// Without this, DOM iteration order determines which sibling gets
|
||||
// the base slug vs ~1, causing non-deterministic output across
|
||||
// different serializations of the same place.
|
||||
let mut sorted_children: Vec<_> = new_inst
|
||||
.children()
|
||||
.iter()
|
||||
.map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone()))
|
||||
.collect();
|
||||
sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b));
|
||||
|
||||
for (new_child_ref, _) in &sorted_children {
|
||||
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
||||
|
||||
// Skip children with duplicate names - cannot reliably sync them
|
||||
|
|
@ -220,10 +272,19 @@ pub fn syncback_dir_no_meta<'sync>(
|
|||
continue;
|
||||
}
|
||||
// This child exists in both doms. Pass it on.
|
||||
children.push(snapshot.with_joined_path(*new_child_ref, Some(old_child.id()))?);
|
||||
let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path(
|
||||
*new_child_ref,
|
||||
Some(old_child.id()),
|
||||
&taken_names,
|
||||
)?;
|
||||
taken_names.insert(dedup_key.to_lowercase());
|
||||
children.push(child_snap);
|
||||
} else {
|
||||
// The child only exists in the the new dom
|
||||
children.push(snapshot.with_joined_path(*new_child_ref, None)?);
|
||||
let (child_snap, _needs_meta, dedup_key) =
|
||||
snapshot.with_joined_path(*new_child_ref, None, &taken_names)?;
|
||||
taken_names.insert(dedup_key.to_lowercase());
|
||||
children.push(child_snap);
|
||||
}
|
||||
}
|
||||
// Any children that are in the old dom but not the new one are removed.
|
||||
|
|
@ -246,7 +307,15 @@ pub fn syncback_dir_no_meta<'sync>(
|
|||
}));
|
||||
} else {
|
||||
// There is no old instance. Just add every child.
|
||||
for new_child_ref in new_inst.children() {
|
||||
// Sort alphabetically for deterministic dedup ordering.
|
||||
let mut sorted_children: Vec<_> = new_inst
|
||||
.children()
|
||||
.iter()
|
||||
.map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone()))
|
||||
.collect();
|
||||
sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b));
|
||||
|
||||
for (new_child_ref, _) in &sorted_children {
|
||||
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
||||
|
||||
// Skip children with duplicate names - cannot reliably sync them
|
||||
|
|
@ -267,7 +336,10 @@ pub fn syncback_dir_no_meta<'sync>(
|
|||
if snapshot.should_ignore_tree(*new_child_ref) {
|
||||
continue;
|
||||
}
|
||||
children.push(snapshot.with_joined_path(*new_child_ref, None)?);
|
||||
let (child_snap, _needs_meta, dedup_key) =
|
||||
snapshot.with_joined_path(*new_child_ref, None, &taken_names)?;
|
||||
taken_names.insert(dedup_key.to_lowercase());
|
||||
children.push(child_snap);
|
||||
}
|
||||
}
|
||||
let mut fs_snapshot = FsSnapshot::new();
|
||||
|
|
|
|||
|
|
@ -35,20 +35,14 @@ pub fn snapshot_json_model(
|
|||
format!("File is not a valid JSON model: {}", path.display())
|
||||
})?;
|
||||
|
||||
if let Some(top_level_name) = &instance.name {
|
||||
let new_name = format!("{}.model.json5", top_level_name);
|
||||
// If the JSON has a name property, preserve it in metadata for syncback
|
||||
let specified_name = instance.name.clone();
|
||||
|
||||
log::warn!(
|
||||
"Model at path {} had a top-level Name field. \
|
||||
This field has been ignored since Rojo 6.0.\n\
|
||||
Consider removing this field and renaming the file to {}.",
|
||||
new_name,
|
||||
path.display()
|
||||
);
|
||||
// Use the name from JSON if present, otherwise fall back to filename-derived name
|
||||
if instance.name.is_none() {
|
||||
instance.name = Some(name.to_owned());
|
||||
}
|
||||
|
||||
instance.name = Some(name.to_owned());
|
||||
|
||||
let id = instance.id.take().map(RojoRef::new);
|
||||
let schema = instance.schema.take();
|
||||
|
||||
|
|
@ -62,7 +56,8 @@ pub fn snapshot_json_model(
|
|||
.relevant_paths(vec![vfs.canonicalize(path)?])
|
||||
.context(context)
|
||||
.specified_id(id)
|
||||
.schema(schema);
|
||||
.schema(schema)
|
||||
.specified_name(specified_name);
|
||||
|
||||
Ok(Some(snapshot))
|
||||
}
|
||||
|
|
@ -81,6 +76,14 @@ pub fn syncback_json_model<'sync>(
|
|||
// schemas will ever exist in one project for it to matter, but it
|
||||
// could have a performance cost.
|
||||
model.schema = old_inst.metadata().schema.clone();
|
||||
model.name = old_inst.metadata().specified_name.clone();
|
||||
}
|
||||
// Filesystem name differs from the instance name (due to slugification
|
||||
// or deduplication). Store the real name so it survives a round-trip.
|
||||
// Checked independently: old_inst may exist but lack a specified_name
|
||||
// (e.g., a previously filesystem-safe name was renamed to need slugification).
|
||||
if model.name.is_none() && snapshot.needs_meta_name {
|
||||
model.name = Some(snapshot.new_inst().name.clone());
|
||||
}
|
||||
|
||||
let serialized = match crate::json::to_vec_pretty_sorted(&model) {
|
||||
|
|
|
|||
|
|
@ -8,9 +8,8 @@ use rbx_dom_weak::{
|
|||
};
|
||||
|
||||
use crate::{
|
||||
path_encoding::encode_path_name,
|
||||
snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot},
|
||||
syncback::{FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
syncback::{name_needs_slugify, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
};
|
||||
|
||||
use super::{
|
||||
|
|
@ -165,10 +164,11 @@ pub fn syncback_lua<'sync>(
|
|||
.or_else(|| file_stem.strip_suffix(".legacy"))
|
||||
.unwrap_or(file_stem);
|
||||
let meta_name = if meta_name.is_empty() {
|
||||
if snapshot.encode_windows_invalid_chars() {
|
||||
encode_path_name(&new_inst.name)
|
||||
let instance_name = &new_inst.name;
|
||||
if name_needs_slugify(instance_name) {
|
||||
slugify_name(instance_name)
|
||||
} else {
|
||||
new_inst.name.clone()
|
||||
instance_name.clone()
|
||||
}
|
||||
} else {
|
||||
meta_name.to_string()
|
||||
|
|
|
|||
|
|
@ -36,6 +36,9 @@ pub struct AdjacentMetadata {
|
|||
#[serde(default, skip_serializing_if = "IndexMap::is_empty")]
|
||||
pub attributes: IndexMap<String, UnresolvedValue>,
|
||||
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub name: Option<String>,
|
||||
|
||||
#[serde(skip)]
|
||||
pub path: PathBuf,
|
||||
}
|
||||
|
|
@ -52,9 +55,8 @@ impl AdjacentMetadata {
|
|||
_name: &str,
|
||||
snapshot: &mut InstanceSnapshot,
|
||||
) -> anyhow::Result<()> {
|
||||
// Use the file stem from the actual path (which has encoded chars)
|
||||
// rather than the decoded name, so meta files are found correctly
|
||||
// when encodeWindowsInvalidChars is enabled.
|
||||
// Use the file stem from the actual path (which may be slugified)
|
||||
// rather than the instance name, so meta files are found correctly.
|
||||
// Strip known script type suffixes to get the base name for the meta file.
|
||||
let file_stem = path.file_stem().and_then(|s| s.to_str()).unwrap_or("");
|
||||
let base_name = file_stem
|
||||
|
|
@ -156,6 +158,20 @@ impl AdjacentMetadata {
|
|||
}
|
||||
}
|
||||
|
||||
let name = snapshot
|
||||
.old_inst()
|
||||
.and_then(|inst| inst.metadata().specified_name.clone())
|
||||
.or_else(|| {
|
||||
// Filesystem name differs from the instance name (due to
|
||||
// slugification or deduplication). Store the real name so
|
||||
// it survives a round-trip through the filesystem.
|
||||
if snapshot.needs_meta_name {
|
||||
Some(snapshot.new_inst().name.clone())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
|
||||
Ok(Some(Self {
|
||||
ignore_unknown_instances: if ignore_unknown_instances {
|
||||
Some(true)
|
||||
|
|
@ -164,6 +180,7 @@ impl AdjacentMetadata {
|
|||
},
|
||||
properties,
|
||||
attributes,
|
||||
name,
|
||||
path,
|
||||
id: None,
|
||||
schema,
|
||||
|
|
@ -225,11 +242,26 @@ impl AdjacentMetadata {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
fn apply_name(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> {
|
||||
if self.name.is_some() && snapshot.metadata.specified_name.is_some() {
|
||||
anyhow::bail!(
|
||||
"cannot specify a name using {} (instance has a name from somewhere else)",
|
||||
self.path.display()
|
||||
);
|
||||
}
|
||||
if let Some(name) = &self.name {
|
||||
snapshot.name = name.clone().into();
|
||||
}
|
||||
snapshot.metadata.specified_name = self.name.take();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn apply_all(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> {
|
||||
self.apply_ignore_unknown_instances(snapshot);
|
||||
self.apply_properties(snapshot)?;
|
||||
self.apply_id(snapshot)?;
|
||||
self.apply_schema(snapshot)?;
|
||||
self.apply_name(snapshot)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
@ -238,11 +270,13 @@ impl AdjacentMetadata {
|
|||
///
|
||||
/// - The number of properties and attributes is 0
|
||||
/// - `ignore_unknown_instances` is None
|
||||
/// - `name` is None
|
||||
#[inline]
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.attributes.is_empty()
|
||||
&& self.properties.is_empty()
|
||||
&& self.ignore_unknown_instances.is_none()
|
||||
&& self.name.is_none()
|
||||
}
|
||||
|
||||
// TODO: Add method to allow selectively applying parts of metadata and
|
||||
|
|
@ -274,6 +308,9 @@ pub struct DirectoryMetadata {
|
|||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub class_name: Option<Ustr>,
|
||||
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub name: Option<String>,
|
||||
|
||||
#[serde(skip)]
|
||||
pub path: PathBuf,
|
||||
}
|
||||
|
|
@ -383,6 +420,20 @@ impl DirectoryMetadata {
|
|||
}
|
||||
}
|
||||
|
||||
let name = snapshot
|
||||
.old_inst()
|
||||
.and_then(|inst| inst.metadata().specified_name.clone())
|
||||
.or_else(|| {
|
||||
// Filesystem name differs from the instance name (due to
|
||||
// slugification or deduplication). Store the real name so
|
||||
// it survives a round-trip through the filesystem.
|
||||
if snapshot.needs_meta_name {
|
||||
Some(snapshot.new_inst().name.clone())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
|
||||
Ok(Some(Self {
|
||||
ignore_unknown_instances: if ignore_unknown_instances {
|
||||
Some(true)
|
||||
|
|
@ -392,6 +443,7 @@ impl DirectoryMetadata {
|
|||
properties,
|
||||
attributes,
|
||||
class_name: None,
|
||||
name,
|
||||
path,
|
||||
id: None,
|
||||
schema,
|
||||
|
|
@ -404,6 +456,7 @@ impl DirectoryMetadata {
|
|||
self.apply_properties(snapshot)?;
|
||||
self.apply_id(snapshot)?;
|
||||
self.apply_schema(snapshot)?;
|
||||
self.apply_name(snapshot)?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -475,17 +528,34 @@ impl DirectoryMetadata {
|
|||
snapshot.metadata.schema = self.schema.take();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn apply_name(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> {
|
||||
if self.name.is_some() && snapshot.metadata.specified_name.is_some() {
|
||||
anyhow::bail!(
|
||||
"cannot specify a name using {} (instance has a name from somewhere else)",
|
||||
self.path.display()
|
||||
);
|
||||
}
|
||||
if let Some(name) = &self.name {
|
||||
snapshot.name = name.clone().into();
|
||||
}
|
||||
snapshot.metadata.specified_name = self.name.take();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Returns whether the metadata is 'empty', meaning it doesn't have anything
|
||||
/// worth persisting in it. Specifically:
|
||||
///
|
||||
/// - The number of properties and attributes is 0
|
||||
/// - `ignore_unknown_instances` is None
|
||||
/// - `class_name` is either None or not Some("Folder")
|
||||
/// - `name` is None
|
||||
#[inline]
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.attributes.is_empty()
|
||||
&& self.properties.is_empty()
|
||||
&& self.ignore_unknown_instances.is_none()
|
||||
&& self.name.is_none()
|
||||
&& if let Some(class) = &self.class_name {
|
||||
class == "Folder"
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -30,7 +30,6 @@ use serde::{Deserialize, Serialize};
|
|||
|
||||
use crate::{
|
||||
glob::Glob,
|
||||
path_encoding::decode_path_name,
|
||||
project::DEFAULT_PROJECT_NAMES,
|
||||
syncback::{SyncbackReturn, SyncbackSnapshot},
|
||||
};
|
||||
|
|
@ -72,19 +71,17 @@ pub fn snapshot_from_vfs(
|
|||
|
||||
if meta.is_dir() {
|
||||
let (middleware, dir_name, init_path) = get_dir_middleware(vfs, path)?;
|
||||
// Decode directory name if the flag is set
|
||||
let decoded_name = if context.decode_windows_invalid_chars {
|
||||
decode_path_name(dir_name)
|
||||
} else {
|
||||
dir_name.to_string()
|
||||
};
|
||||
// The directory name is used as-is from the filesystem.
|
||||
// If a different instance name is desired, it comes from the
|
||||
// `name` field in init.meta.json / init.meta.json5 (applied later
|
||||
// by DirectoryMetadata::apply_name).
|
||||
// TODO: Support user defined init paths
|
||||
// If and when we do, make sure to go support it in
|
||||
// `Project::set_file_name`, as right now it special-cases
|
||||
// `default.project.json5` as an `init` path.
|
||||
match middleware {
|
||||
Middleware::Dir => middleware.snapshot(context, vfs, path, &decoded_name),
|
||||
_ => middleware.snapshot(context, vfs, &init_path, &decoded_name),
|
||||
Middleware::Dir => middleware.snapshot(context, vfs, path, dir_name),
|
||||
_ => middleware.snapshot(context, vfs, &init_path, dir_name),
|
||||
}
|
||||
} else {
|
||||
let file_name = path
|
||||
|
|
@ -166,23 +163,17 @@ fn snapshot_from_path(
|
|||
vfs: &Vfs,
|
||||
path: &Path,
|
||||
) -> anyhow::Result<Option<InstanceSnapshot>> {
|
||||
// Helper to decode name if the flag is set
|
||||
let decode_name = |name: &str| -> String {
|
||||
if context.decode_windows_invalid_chars {
|
||||
decode_path_name(name)
|
||||
} else {
|
||||
name.to_string()
|
||||
}
|
||||
};
|
||||
|
||||
// File names are used as-is from the filesystem. If a different instance
|
||||
// name is needed (e.g. for names with special chars), it comes from the
|
||||
// `name` field in adjacent .meta.json / .model.json files.
|
||||
if let Some(rule) = context.get_user_sync_rule(path) {
|
||||
let name = decode_name(rule.file_name_for_path(path)?);
|
||||
return rule.middleware.snapshot(context, vfs, path, &name);
|
||||
let name = rule.file_name_for_path(path)?;
|
||||
return rule.middleware.snapshot(context, vfs, path, name);
|
||||
} else {
|
||||
for rule in default_sync_rules() {
|
||||
if rule.matches(path) {
|
||||
let name = decode_name(rule.file_name_for_path(path)?);
|
||||
return rule.middleware.snapshot(context, vfs, path, &name);
|
||||
let name = rule.file_name_for_path(path)?;
|
||||
return rule.middleware.snapshot(context, vfs, path, name);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,7 +20,10 @@ use crate::{
|
|||
PathIgnoreRule, SyncRule,
|
||||
},
|
||||
snapshot_middleware::Middleware,
|
||||
syncback::{filter_properties, inst_path, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
|
||||
syncback::{
|
||||
filter_properties, inst_path, name_needs_slugify, slugify_name, strip_middleware_extension,
|
||||
FsSnapshot, SyncbackReturn, SyncbackSnapshot,
|
||||
},
|
||||
variant_eq::variant_eq,
|
||||
RojoRef,
|
||||
};
|
||||
|
|
@ -647,7 +650,55 @@ pub fn syncback_project<'sync>(
|
|||
|
||||
// All of the children in this loop are by their nature not in the
|
||||
// project, so we just need to run syncback on them.
|
||||
for (name, new_child) in new_child_map.drain() {
|
||||
//
|
||||
// Track taken names (bare slugs) per parent directory so siblings
|
||||
// under the same parent deduplicate against each other.
|
||||
let mut taken_names_per_dir: std::collections::HashMap<
|
||||
std::path::PathBuf,
|
||||
std::collections::HashSet<String>,
|
||||
> = std::collections::HashMap::new();
|
||||
|
||||
// Pre-seed taken_names from old children's actual filesystem dedup keys
|
||||
// so that new-only children correctly dedup against existing siblings.
|
||||
// We derive keys from relevant_paths (the real filename on disk) rather
|
||||
// than slugifying instance names, because an old instance may already
|
||||
// have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare
|
||||
// slug wouldn't capture.
|
||||
// Only in incremental mode: in clean mode, old_ref is forced to None
|
||||
// for all children, so every child is treated as new and pre-seeding
|
||||
// would cause false collisions (spurious ~1 suffixes).
|
||||
if snapshot.data.is_incremental() {
|
||||
if let Some(parent_path) = ref_to_path_map.get(&new_inst.referent()) {
|
||||
let taken = taken_names_per_dir.entry(parent_path.clone()).or_default();
|
||||
for old_child in old_child_map.values() {
|
||||
if let Some(path) = old_child.metadata().relevant_paths.first() {
|
||||
if let Some(filename) = path.file_name().and_then(|f| f.to_str()) {
|
||||
let middleware =
|
||||
old_child.metadata().middleware.unwrap_or(Middleware::Dir);
|
||||
let dedup_key =
|
||||
strip_middleware_extension(filename, middleware).to_lowercase();
|
||||
taken.insert(dedup_key);
|
||||
}
|
||||
} else {
|
||||
// Fallback (shouldn't happen for old instances with disk presence)
|
||||
let name = old_child.name();
|
||||
let slug = if name_needs_slugify(name) {
|
||||
slugify_name(name).to_lowercase()
|
||||
} else {
|
||||
name.to_lowercase()
|
||||
};
|
||||
taken.insert(slug);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Sort remaining children by name for deterministic dedup ordering.
|
||||
// Without this, HashMap iteration order determines which sibling gets
|
||||
// the base slug vs ~1, causing non-deterministic output across runs.
|
||||
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 {
|
||||
// Skip instances of ignored classes
|
||||
if snapshot.should_ignore_class(&new_child.class) {
|
||||
// Also remove from old_child_map so it won't be marked as removed
|
||||
|
|
@ -680,11 +731,15 @@ pub fn syncback_project<'sync>(
|
|||
// concern with directories because they're singular things,
|
||||
// files that contain their own children.
|
||||
if parent_middleware != Middleware::Project {
|
||||
descendant_snapshots.push(snapshot.with_base_path(
|
||||
let taken_names = taken_names_per_dir.entry(parent_path.clone()).or_default();
|
||||
let (child_snap, _needs_meta, dedup_key) = snapshot.with_base_path(
|
||||
&parent_path,
|
||||
new_child.referent(),
|
||||
None,
|
||||
)?);
|
||||
taken_names,
|
||||
)?;
|
||||
taken_names.insert(dedup_key.to_lowercase());
|
||||
descendant_snapshots.push(child_snap);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.csv
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ snapshot_id: "00000000000000000000000000000000"
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
instigating_source:
|
||||
Path: /root
|
||||
Path: /root/init.csv
|
||||
relevant_paths:
|
||||
- /root
|
||||
- /root/init.luau
|
||||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /root/init.server.lua
|
||||
- /root/init.client.lua
|
||||
- /root/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ snapshot_id: "00000000000000000000000000000000"
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
instigating_source:
|
||||
Path: /root
|
||||
Path: /root/init.csv
|
||||
relevant_paths:
|
||||
- /root
|
||||
- /root/init.luau
|
||||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /root/init.server.lua
|
||||
- /root/init.client.lua
|
||||
- /root/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.csv
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /foo/init.server.lua
|
||||
- /foo/init.client.lua
|
||||
- /foo/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /foo/init.server.lua
|
||||
- /foo/init.client.lua
|
||||
- /foo/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
@ -45,8 +44,7 @@ children:
|
|||
- /foo/Child/init.server.lua
|
||||
- /foo/Child/init.client.lua
|
||||
- /foo/Child/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: dir
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.json5
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.json5
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo.model.json5
|
||||
relevant_paths:
|
||||
- /foo.model.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
@ -24,8 +23,7 @@ children:
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo.model.json5
|
||||
relevant_paths:
|
||||
- /foo.model.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
@ -24,8 +23,7 @@ children:
|
|||
metadata:
|
||||
ignore_unknown_instances: false
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.client.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /root/init.server.lua
|
||||
- /root/init.client.lua
|
||||
- /root/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -19,8 +19,7 @@ metadata:
|
|||
- /root/init.server.lua
|
||||
- /root/init.client.lua
|
||||
- /root/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.legacy.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.local.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.plugin.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /bar.server.luau
|
||||
- /bar.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.server.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo.server.luau
|
||||
- /foo.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
- /foo/bar.meta.json
|
||||
- /foo/bar.meta.jsonc
|
||||
- /foo/bar.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -7,8 +7,7 @@ metadata:
|
|||
ignore_unknown_instances: false
|
||||
relevant_paths:
|
||||
- /foo/bar.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
- /foo/bar.meta.json
|
||||
- /foo/bar.meta.jsonc
|
||||
- /foo/bar.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
- /foo/init.meta.json
|
||||
- /foo/init.meta.jsonc
|
||||
- /foo/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -7,8 +7,7 @@ metadata:
|
|||
ignore_unknown_instances: false
|
||||
relevant_paths:
|
||||
- /foo/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
- /foo/init.meta.json
|
||||
- /foo/init.meta.jsonc
|
||||
- /foo/init.meta.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: manually specified
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo/default.project.json5
|
||||
relevant_paths:
|
||||
- /foo/default.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo/hello.project.json5
|
||||
relevant_paths:
|
||||
- /foo/hello.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo/other.project.json5
|
||||
- /foo/default.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: project
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo.project.json5
|
||||
relevant_paths:
|
||||
- /foo.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
@ -29,8 +28,7 @@ children:
|
|||
$className: Model
|
||||
parent_class: Folder
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo/other.project.json5
|
||||
- /foo/default.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: project
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -10,8 +10,7 @@ metadata:
|
|||
relevant_paths:
|
||||
- /foo/other.project.json5
|
||||
- /foo/default.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: project
|
||||
schema: ~
|
||||
|
|
@ -30,8 +29,7 @@ children:
|
|||
$className: Model
|
||||
parent_class: Folder
|
||||
relevant_paths: []
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -11,8 +11,7 @@ metadata:
|
|||
- /foo/other.txt
|
||||
- /foo/other.meta.json5
|
||||
- /foo/default.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: text
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo.project.json5
|
||||
relevant_paths:
|
||||
- /foo.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
|
|
@ -9,8 +9,7 @@ metadata:
|
|||
Path: /foo.project.json5
|
||||
relevant_paths:
|
||||
- /foo.project.json5
|
||||
context:
|
||||
decode_windows_invalid_chars: true
|
||||
context: {}
|
||||
specified_id: ~
|
||||
middleware: ~
|
||||
schema: ~
|
||||
|
|
|
|||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Add table
Add a link
Reference in a new issue