Conversation
There was a problem hiding this comment.
🟡 Changes recommended
UgcImportHostBridge.TryImport can mutate the game’s shared import selection before verifying required symbols exist, leaving behind overrides on early failure paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds support for loading a local Robotopia Creator world export (.roboworld / .json / .json.gz) by bridging into the game’s own import host (build 2409) rather than reimplementing the importer, with explicit folder-confinement rules and unit tests to pin that security boundary.
Changes:
- Introduces
UgcImportHostBridge(reflection-based) +RoboWorldImportPlan(Unity-free policy) to import local exports via the game’s importer. - Wires local-world configuration (
enableLocalWorlds,localWorldFolder) intoWorldsServiceandWorldsMod, including proper disposal. - Adds ModManager harness coverage for the folder-confinement rules; updates docs/changelog and adds the binding manifest + refreshed surface baseline.
File summaries
| File | Description |
|---|---|
| tests/TopiaForge.ModManager.Tests/TopiaForge.ModManager.Tests.csproj | Adds RoboWorldImportPlan to the ModManager test compile set. |
| tests/TopiaForge.ModManager.Tests/RoboWorldImportPlanTests.cs | Adds unit tests pinning folder confinement + extension handling. |
| tests/TopiaForge.ModManager.Tests/Program.cs | Registers the new test harness entry point. |
| mods/TopiaForge.Worlds/WorldsService.Sessions.cs | Disposes local-world bridge during service teardown. |
| mods/TopiaForge.Worlds/WorldsService.LocalWorlds.cs | Adds local export listing + import entry points and config plumbing. |
| mods/TopiaForge.Worlds/WorldsMod.cs | Wires config values into WorldsService on load. |
| mods/TopiaForge.Worlds/WorldsConfig.cs | Adds enableLocalWorlds and localWorldFolder config surface + defaults. |
| mods/TopiaForge.Worlds/UgcImportHostBridge.cs | New reflection bridge to the game’s import host (scan/validate/import). |
| mods/TopiaForge.Worlds/RoboWorldImportPlan.cs | New Unity-free planning policy for confinement + supported extensions. |
| mods/TopiaForge.Worlds/CHANGELOG.md | Documents the new local-world import capability and config keys. |
| docs/CustomWorlds.md | Documents local .roboworld loading behavior and security boundary. |
| docs/CreatorScope.md | Updates creator-scope status to reflect local import support. |
| bindings/io.github.furroxide.topiaforge.worlds.gamebindings.json | Declares new degraded bindings used by the import host bridge. |
| baselines/gamecode.surface.baseline.json | Refreshes the captured gamecode surface baseline metadata/content hash. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ection `TryImport` applied the config override first and looked up `ConfigureRuntimeImportFolder` and `ImportFile` after. A build that no longer exposes either one therefore refused the import having already written our folder and file into `UgcImportHostConfig` — a ScriptableObject the game's own import host reads. The override was only undone at `Dispose`, so for the rest of the session the game's importer would have been pointed at us by a call that reported failure. Both symbols are now resolved before anything is written, so that path refuses without having touched the game's state at all. The two failures that can only happen after the write — the importer producing no scene, and an exception out of the reflection calls — restore immediately instead of waiting for disposal. `Dispose` and those paths share one `RestoreConfigSelection`, which clears the snapshot so a later import re-snapshots rather than restoring stale values. A successful import deliberately keeps the override: the controller re-reads its config on `OnEnable` and would otherwise pull the folder back to the game's default on the next scene activation. That is unchanged. Also corrects `RoboWorldImportPlan.TryPlan`'s `folderPath` doc, which claimed the folder "Must exist". It is never probed: confinement is a string comparison, and a folder that does not exist cannot hold a file, so the injected `fileExists` already reports a missing one. Both reported by Copilot on #67.
d52fe85 to
96777e3
Compare
…ection `TryImport` applied the config override first and looked up `ConfigureRuntimeImportFolder` and `ImportFile` after. A build that no longer exposes either one therefore refused the import having already written our folder and file into `UgcImportHostConfig` — a ScriptableObject the game's own import host reads. The override was only undone at `Dispose`, so for the rest of the session the game's importer would have been pointed at us by a call that reported failure. Both symbols are now resolved before anything is written, so that path refuses without having touched the game's state at all. The two failures that can only happen after the write — the importer producing no scene, and an exception out of the reflection calls — restore immediately instead of waiting for disposal. `Dispose` and those paths share one `RestoreConfigSelection`, which clears the snapshot so a later import re-snapshots rather than restoring stale values. A successful import deliberately keeps the override: the controller re-reads its config on `OnEnable` and would otherwise pull the folder back to the game's default on the next scene activation. That is unchanged. Also corrects `RoboWorldImportPlan.TryPlan`'s `folderPath` doc, which claimed the folder "Must exist". It is never probed: confinement is a string comparison, and a folder that does not exist cannot hold a file, so the injected `fileExists` already reports a missing one. Both reported by Copilot on #67.
96777e3 to
39b4dc4
Compare
Build 2409's importer has two reachable halves that need no account: UgcImportHostConfig holds the serialized ImportFolderOverride / SelectedExportFilePath / SelectedSceneId selection, and UgcImportHostSceneController exposes ConfigureRuntimeImportFolder and ImportFile, both public and both taking nothing but a path. That is a complete local load path — a world a player exported from the Creator and has sitting on disk can be brought into the running game with no Discord sign-in, no publish, and no backend call. Worlds now uses it. The bridge deliberately touches none of the cloud entry points — not UgcPublishedProjectLoader, not UgcAutomergeSyncClient, not UgcLaunchUrlStartup — so "this is local only" is true by construction rather than by promise, and a reviewer can check it by grep. The importer accepts any path, which makes the folder the trust boundary: a caller that could name an arbitrary path could make the game read any file on the machine and report parse errors about its contents. So an import is confined to one declared folder, and a request outside it is refused before the extension and existence checks, because otherwise the refusal text would leak which files exist elsewhere. Confinement compares against the separator-terminated folder, so "…\worlds-backup\x" cannot pass as a child of "…\worlds". Three details that are easy to get wrong and are pinned by tests: - .json.gz is a compound suffix. A GetExtension comparison reads it as .gz and refuses the compressed exports the Creator actually hands out. - ImportFile returns void and swallows its own failures, so "it was called" is not "it worked". The import is confirmed by reading LastImportedScene, and a null there is reported as a failure rather than a load. - UgcImportHostConfig is a shipped ScriptableObject shared with the game's own import host. The previous selection is snapshotted and restored on unload; leaving a TopiaForge folder in it would silently change what the game does next. Writing the config as well as calling ConfigureRuntimeImportFolder is what survives the controller's OnEnable, which re-reads the config. An export is parsed with the game's own UgcExportLoader before the scene is touched, so a malformed file is refused while the current world is still intact and the reason the player sees is the game's wording, not ours. The folder listing keeps unreadable files and shows the scanner's error for each: a player whose export is missing from a list learns nothing. 21 new bindings, all Degraded — if a future build moves the importer, local worlds stop loading with a stated reason and nothing else in Worlds changes. `gamecompat verify` resolves all of them against 2409 (203 declared, 182 verifiable, 0 errors, 0 warnings); the surface baseline is refreshed and its diff is five added types, no removals and no changes. The path rules live in a Unity-free RoboWorldImportPlan compiled into the offline harness, so the trust boundary is unit-tested without a game install. Registering a local world as a gamemode arena is the next step; this commit stops at loading one.
…ection `TryImport` applied the config override first and looked up `ConfigureRuntimeImportFolder` and `ImportFile` after. A build that no longer exposes either one therefore refused the import having already written our folder and file into `UgcImportHostConfig` — a ScriptableObject the game's own import host reads. The override was only undone at `Dispose`, so for the rest of the session the game's importer would have been pointed at us by a call that reported failure. Both symbols are now resolved before anything is written, so that path refuses without having touched the game's state at all. The two failures that can only happen after the write — the importer producing no scene, and an exception out of the reflection calls — restore immediately instead of waiting for disposal. `Dispose` and those paths share one `RestoreConfigSelection`, which clears the snapshot so a later import re-snapshots rather than restoring stale values. A successful import deliberately keeps the override: the controller re-reads its config on `OnEnable` and would otherwise pull the folder back to the game's default on the next scene activation. That is unchanged. Also corrects `RoboWorldImportPlan.TryPlan`'s `folderPath` doc, which claimed the folder "Must exist". It is never probed: confinement is a string comparison, and a folder that does not exist cannot hold a file, so the injected `fileExists` already reports a missing one. Both reported by Copilot on #67.
39b4dc4 to
abbcf21
Compare
|
Unity PR artifacts are ready.
Workflow run: Unity Source Validation #193 Artifacts are temporary and expire according to the workflow retention policy. |
`Build guides, C# and Dart references, and search` fails on this branch: docs/CustomWorlds.md: local documentation link is not published by Starlight: CreatorScope.md Custom worlds is a published guide; `docs/CreatorScope.md` is not in the website catalog, so on the site that link would 404. Every other published->unpublished link in the tree is the other way round — only unpublished pages link to unpublished pages. The sentence wants the Creator, not an internal scope memo about it, and `docs/CreatorCompanion.md` already spells that exact phrase as `official [Robotopia Creator](https://robotopia.gg/editor/)`. Reusing it fixes the link for site readers and improves it for repository readers, without deciding whether the scope page belongs on the public site. Publishing `CreatorScope.md` instead is a two-line alternative — a catalog entry and a sidebar slot — but it also needs a call on the one `Harmony` mention in its table, which the public-contract guard forbids. That is a docs-IA decision rather than a build fix, so it is left open.
abbcf21 to
b9b07cd
Compare
…irement (#84) ## Summary Two things that belong together: the local `.roboworld` import becomes something a mod can actually call, and the residue the UGC live-sync retirement left behind is cleared. **This supersedes #67.** Its three commits are included here, rebased onto `dev` and otherwise unchanged. #67 landed the import complete, tested and documented, but `TryLoadLocalWorld` and `ListLocalWorlds` were `internal` with zero callers anywhere in the tree, so nothing could reach it. Close #67 in favour of this, or merge it first and this rebases to nothing. ## What is here | | | | --- | --- | | 1-3 | #67 as it stands: load a local export through the game's own import host | | 4 | Bootstrap stops restoring the deleted sidecar | | 5 | Launcher stops advertising the removed Go Live flow | | 6 | Asset template stops scaffolding a decoy Unity folder | | 7 | Stale references across 14 files | | 8 | Asset overrides restored on the import path | | 9 | Local world import promoted onto the contract | | 10 | Bootstrap stops overwriting a configured hooks path | ### The one that was actually broken `tools/bootstrap-dev.ps1` still ran `npm ci` against the deleted `tools/ugc-automerge-sidecar`. The loop hits `website` first and succeeds, then fails its lockfile guard on the second entry and throws under `$ErrorActionPreference = "Stop"`, before `$websiteRestored` is ever set. Every contributor with Node and npm installed hit this on the plain path documented in the README, not just `-Verify`. No CI job executes the script, which is why the gates stayed green while onboarding was broken. ### Asset overrides The retirement removed the only way for a mod's prefab to stand in for an authored asset id. The game still owns the mechanism: `UgcRuntimeAssetConfig.SetRuntimeOverride`, `ClearRuntimeOverrides` and the `runtimeOverrides` table are all public and all present on 2409, and the importer consults that table while resolving each entity. This fills the table the game already reads rather than rebuilding a resolver. Placement is load-bearing: overrides are applied after the config selection and **before** `ImportFile`, because the importer resolves every asset id while it builds the scene and offers no way to re-skin an entity afterwards. A refused import clears them again. Three new bindings, none Critical - the `RuntimeAssetConfig` property is `Degraded`, the two override methods are `Optional`. Without them a world still imports, just with the game's own asset, so `verify` cannot fail on them by construction. ## Breaking changes `IWorldGamemodeService` gains three members: `RegisterAssetOverride`, `ListLocalWorlds` and `LoadLocalWorld`. Third-party implementers must add them. `TopiaForge.Mods.Worlds` and `TopiaForge.Mods.Testing` baselines are refreshed accordingly. Nothing in `TopiaForge.Mods.Abstractions`' other contracts moved. ## Gates Run locally against an installed 2409 build. | Gate | Result | | --- | --- | | `TopiaForge.slnx` Release build | pass - 0 warnings, 0 errors | | `tools/verify-csharp-release-surface.sh` | pass - 11 SDK packages, all 7 harnesses | | `gamecompat audit --strict` | pass - 0 problems, 0 undeclared, 0 stale | | `gamecompat verify` | pass - 206 declared, 185 verifiable, **0 errors, 0 warnings** (up exactly three) | | `launcher_domain` / `launcher_data` / `topiaforge_cli` | 197 / 347 / 211, zero failures | | `launcher_ui` / `topiaforge_launcher_flutter` | 3 / 51, zero failures | | PSScriptAnalyzer over `tools/` | 0 findings | | Residue audit, markdown links | pass | Both new tests were checked with a deliberate failure to confirm they execute rather than passing vacuously. ## Notes for review - `website/docfx.json` listed a deleted project. It is removed here, but it was **not** breaking the docs job - every run since the retirement is green, so DocFX skips the unresolved entry. Hygiene, not a fix. - `docs/ReleaseChecklist.md` already said "Two" VPM packages; the stale "three" was in `ArchitectureInventory.md` and `LaunchBlockers.md`. - Rows in `LaunchBlockers.md` that deliberately record the retirement are left alone. - `dotnet format --verify-no-changes` cannot pass on Windows for this repo - no `.editorconfig`, `core.autocrlf=false`, so it reports 10 phantom whitespace findings in untouched files. CI runs `ubuntu-latest` and is unaffected. --------- Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
You can point Worlds at a
.roboworldfile on disk and have the game load it. The loading is the game's own import host doing the work — I'm not parsing the format, just driving the thing that already knows how.Stacked on #66, so read that one first; the diff here is the single commit on top. I kept it separate because it's new surface rather than a retarget — a new bridge class, a new bindings manifest, about 395 lines of refreshed GameCode baseline — and I didn't want that buried in a fifteen-commit rebase. It also can't move earlier in the stack: it edits
docs/CreatorScope.md, and the commit right before it is the one that creates that file via rename.The code
Two new files under
mods/TopiaForge.Worlds.RoboWorldImportPlan.csdecides whether a given file is allowed to be imported at all; it holds no Unity or GameCode references, so the rules get unit-tested offline (RoboWorldImportPlanTests).UgcImportHostBridge.csis the part that actually touches the game — resolves the import host's symbols, overrides its folder and file selection, callsImportFile, and puts the selection back the way it found it when the provider unloads.The folder is the trust boundary here.
UgcImportHostConfigwill happily take any absolute path, so anything outside the configured folder gets refused before we check whether it exists or has a usable extension — otherwise the refusal message turns into a "does this file exist on your disk" oracle. A sibling folder that merely shares a name prefix doesn't count as inside.Two other bits worth a look while reviewing. The export is parsed with the game's own loader before the scene is touched, so a malformed file fails while the current world is still up, and the player sees the game's wording rather than mine. And
ImportFilereturnsvoidand swallows its own failures, so afterwards we checkLastImportedScene— "we called it" isn't "it worked".Config is two keys,
enableLocalWorldsandlocalWorldFolder(empty means the folder the game itself scans). Every binding on this path isDegraded, so if a later build moves the import host, local worlds stop loading with a stated reason and nothing else in Worlds changes behaviour. The rest of the diff is wiring and docs:WorldsMod,WorldsConfig,WorldsService.LocalWorlds,WorldsService.Sessions, the new binding manifest, a refreshedbaselines/gamecode.surface.baseline.json, and updates todocs/CustomWorlds.md,docs/CreatorScope.mdand the mod changelog.Gates
The full run I posted on #66 was executed against this branch's head, so those numbers already cover this commit. What's specific to the new bindings:
gamecompat audit --strict— clean: 0 manifest problems, 0 undeclared, 0 stalegamecompat verify— all 203 declared bindings resolve. 182 verifiable, 21 uncheckable offline, 0 indeterminate, 0 errors, 0 warningsRoboWorldImportPlanTestsrides along in theModManagerharness, which passes with the other sixUsual caveat on that verify number: it says everything this commit binds to resolves against the installed 2409 build. It doesn't say nothing else in 2409 moved.