Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions extensions/renderers/unity/scripts/BuildMacOSPlayer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ public static void Build()
// player can runtime-spawn actors for any campaign (not just the baked scene's cast).
EnsurePackaged();

// Kit rooms (build_room_kit.cs) are QA constructions; a capture flow that saves the scene while
// one exists would otherwise ship it inside the player, drawing grey kit masses over every plate.
string[] strippedQARoots = StripQAConstructions();

// --- Player identity (was DefaultCompany/WorldOS-Unity-spike) ---
PlayerSettings.companyName = "worldos";
PlayerSettings.productName = "WorldOSPlayer";
Expand Down Expand Up @@ -259,13 +263,44 @@ public static void Build()
"platform=" + s.platform + "\n" +
"architecture=" + archResult + "\n" +
"alwaysIncludedShaders=" + string.Join(",", includedShaders) + "\n" +
"strippedQARoots=" + (strippedQARoots.Length == 0 ? "(none)" : string.Join(",", strippedQARoots)) + "\n" +
"scenesBuilt=" + string.Join(",", options.scenes) + "\n");

Debug.Log("[BuildMacOSPlayer] DONE result=" + s.result + " errors=" + s.totalErrors
+ " warnings=" + s.totalWarnings + " size=" + s.totalSize + " time=" + s.totalTime
+ " report=" + reportPath);
}

// QA-construction roots that must NEVER ship inside a player build. build_room_kit.cs assembles
// kit rooms as "KitRoom_<roomId>" roots in whatever scene is open; a capture/lighting flow that
// saves the scene mid-session bakes them in, and the built player then renders grey kit masses
// (fallback boxes, brazier plinths, parapets) in front of every plate. Measured three times
// (kit-crypt cleankit trap ×2, kit-tavern 2026-07-23 — the withheld tavern install). The build
// opens the canonical scene EXPLICITLY (also killing the wrong-open-scene trap), strips matching
// roots, saves, and reports them in build-report.txt (strippedQARoots=...).
const string QARootPrefix = "KitRoom_";

static string[] StripQAConstructions()
{
var scene = UnityEditor.SceneManagement.EditorSceneManager.OpenScene(SceneToBuild);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: OpenScene(path) defaults to OpenSceneMode.Single and discards an unsaved current editor scene without a guard

EditorSceneManager.OpenScene(SceneToBuild) with no mode uses OpenSceneMode.Single. This Build() MenuItem runs in the headed editor (the box forbids -batchmode, per the class doc), so if the operator has an unsaved scene open when they invoke Tools/WorldOS/Build/macOS Player, Unity raises the 'Unsaved changes in scene' dialog and the build blocks until dismissed — or, depending on editor settings, silently discards the operator's unsaved work in the open scene. OccluderVerify.cs:41 and W5bWireScene.cs:35 both call OpenScene(path, OpenSceneMode.Single) explicitly; at minimum make the mode explicit here for parity. More importantly, there is no Force/Save check, so this Build entry point can clobber operator work in a different open scene. Consider OpenSceneMode.Single plus a documented precondition or a SaveCurrentModifiedScenesIfUserWantsTo prompt before opening.

Category: Data loss

Why this matters: Build is an operator-facing MenuItem invoked from the headed editor. An unexpected scene swap mid-build is a data-loss surface for whatever the operator was editing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: StripQAConstructions opens the canonical scene in Single mode, dropping the editor's active scene

StripQAConstructions calls EditorSceneManager.OpenScene(SceneToBuild) with no mode argument, which defaults to OpenSceneMode.Single — it replaces the editor's currently active scene. If a developer invokes Tools/WorldOS/Build/macOS Player (Universal) while editing a different scene with unsaved work, the build silently closes that scene. Use OpenSceneMode.Additive for the strip pass (and remove the stripped roots + the added scene afterward), or document that the build must be run from a clean state. The comment claims this 'opens the canonical scene EXPLICITLY (also killing the wrong-open-scene trap)' but the cost is destroying unsaved editor state on every invocation that hits the build path.

Category: Runtime correctness

Why this matters: A build menu item that silently discards the developer's active scene and unsaved changes is a real workflow hazard on a headed-editor build flow (the BOX.md-forbidden -batchmode path means this runs interactively).

var stripped = new List<string>();
foreach (var go in scene.GetRootGameObjects())
{
if (go != null && go.name.StartsWith(QARootPrefix, StringComparison.Ordinal))
{
stripped.Add(go.name);
UnityEngine.Object.DestroyImmediate(go);
}
}
if (stripped.Count > 0)
{
UnityEditor.SceneManagement.EditorSceneManager.SaveScene(scene);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: StripQAConstructions ignores SaveScene return value; a silent save failure ships contaminated art with strippedQARoots=(none)

EditorSceneManager.SaveScene returns bool and fails non-throwing on read-only files, VCS/OneDrive locks, disk pressure, or a scene path that resolves but is not writable. Here the bool is discarded, so after DestroyImmediate has already mutated the in-memory scene, a failed save (a) leaves the canonical M1CombatV1_canonical.unity dirty on disk as built and (b) still returns the stripped list, so build-report.txt records strippedQARoots= while the on-disk/built scene still contains the roots. The build then proceeds at line 250 against the un-stripped source. Wrap the save: if (!EditorSceneManager.SaveScene(scene)) { Debug.LogError("[BuildMacOSPlayer] SaveScene FAILED after stripping " + SceneToBuild + " — aborting to avoid shipping QA roots"); return; } (or throw) so a failed strip is a hard build stop, not a silent partial.

Category: Release regression

Why this matters: The entire value of this gate is that no KitRoom_* ships. A silent save failure produces a build-report that claims success while the player renders the grey kit masses the PR exists to prevent — the exact kit-tavern contamination, just with a misleading evidence trail.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Strip persists into the tracked source asset as a build side-effect; no smoke/rollback note

SaveScene writes the strip back into Assets/Scenes/M1CombatV1_canonical.unity — a tracked source asset — as an irreversible side effect of Build(). After Build, the canonical scene on disk is permanently altered (the KitRoom_* roots are gone), and a subsequent git status will show the scene changed by a build run. The repo profile asks that scene changes carry explicit rollback and smoke notes; this PR's body should state the strip is destructive to the source scene and that BuildMacOSPlayer is the only recovery path (re-run build_room_kit to re-add, which is the opposite of the gate's intent). Consider writing a timestamped .unity.bak before SaveScene so an operator who ran Build against a scene they did not want stripped can recover without git surgery.

Category: Unity scene/prefab

Why this matters: A build mutating a tracked scene asset without a backup couples release packaging to source-tree mutation; an operator who triggers Build expecting packaging-only behavior silently commits a scene diff.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: StripQAConstructions persists a source-controlled scene file as a build side effect

When stripped.Count > 0 the method calls EditorSceneManager.SaveScene(scene), writing Assets/Scenes/M1CombatV1_canonical.unity — a source-controlled canonical asset — during a build. This can produce an unexpected diff in the scene file (Unity may rewrite metadata/object ordering) that gets committed by accident, and there is no rollback note in the PR despite the repo's high-risk-paths policy requiring explicit rollback notes for Assets/** changes. Prefer destroying the roots in-memory only (the BuildPlayer call at line 250 opens its own copy of the scene from disk for the actual build) and NOT saving the source scene — the build does not require the source .unity to be re-saved, since BuildPlayer serializes from the loaded scene state.

Category: Release regression

Why this matters: Silent mutation of a canonical scene asset during a build is exactly the class of release-regression risk the repo profile flags as high-risk; an unintended scene diff could ship gameplay-changing object state.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: StripQAConstructions persists a destructive SaveScene on the canonical scene asset at build time

StripQAConstructions() opens Assets/Scenes/M1CombatV1_canonical.unity, DestroyImmediate's every root whose name starts with "KitRoom_", then unconditionally calls EditorSceneManager.SaveScene(scene). This mutates the tracked canonical .unity source asset as a side effect of running the build menu item, and the strip predicate is a bare prefix match (QARootPrefix = "KitRoom_") with no allowlist. Today the kit lighting helpers (KitRoom_Fire/KitRoom_TombGlow/KitRoom_CoolKey in build_room_kit.cs:554/561/567) are nested children of the room root, so they are not root objects and survive — but if any future legitimate GameObject is ever placed at the scene ROOT with a KitRoom_ name, this build step will silently destroy it AND commit that deletion to source. The qa_sandbox.py detector (line 99) already maintains a whitelist for those helper prefixes; the stripper does not, so the gate and the stripper are asymmetric. Consider (a) narrowing the strip to the exact contaminant pattern (e.g. KitRoom_) or mirroring the qa_sandbox whitelist, and (b) only saving when a strip actually occurred is already done — but add a one-line guard/comment that this intentionally rewrites the source scene, so the destructive write is not mistaken for a build-output artifact.

Category: Data loss

Why this matters: A build step that silently rewrites the canonical gameplay scene asset is a release-regression/data-loss surface: a contaminant-removal intended for build output instead commits a structural edit to the source-of-truth scene, and the broad prefix makes future false-positive destruction likely.

Debug.LogWarning("[BuildMacOSPlayer] stripped QA construction roots from " + SceneToBuild
+ ": " + string.Join(",", stripped));
}
return stripped.ToArray();
}

// #1674: shaders CombatSurfaceClient resolves at runtime via Shader.Find and that NO asset references, so
// the player build strips them unless they are listed in Graphics -> Always-Included Shaders. The player
// build MUST carry both or the runtime feature (occluder proxies / walk-behind silhouette) silently no-ops
Expand Down
28 changes: 28 additions & 0 deletions qa/qa_sandbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,26 @@ def _meta_path(run: str) -> Path:
return _rundir(run) / "sandbox.json"


def _qa_roots_in_app(app: Path) -> set:
"""Names of QA kit-scene roots (KitRoom_*) baked into the app's level files.

build_room_kit.cs assembles kit rooms in the open scene; a capture flow that saves the scene
bakes them into the next build, which then draws grey kit masses in front of every plate.
Unity level files store GameObject names as plain bytes, so a byte scan is a reliable,
dependency-free detector (the same check as `strings level0 | grep KitRoom_`).
"""
import re
found: set = set()
data_dir = app / "Contents" / "Resources" / "Data"
for lvl in sorted(data_dir.glob("level*")):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: level glob can match non-scene files; byte-scan over-reads arbitrary level-manager/shared files*

data_dir.glob('level*') matches level0, level1, ... but also any file beginning with 'level' that Unity may place under Resources/Data (e.g. shared level data or auxiliary 'level'-prefixed assets). The regex KitRoom_[A-Za-z0-9_]+ is applied to every match via read_bytes(), so a non-scene file that happens to contain the byte sequence 'KitRoom_' (e.g. a debug string, a prefab reference, a sharedAssets blob) would be flagged. The current strip already removes roots from the built scene, so this is low-impact, but tightening the glob to the real Unity scene level naming (e.g. glob('level*') restricted to files with no extension and a leading 'level' + digit) or to mainDataFile/scene-info would make the detector authoritative rather than heuristic.

Category: Flaky test risk

Why this matters: A heuristic byte scan over broadened file set increases false-positive surface on the gate that blocks the QA lane.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Contamination byte-scan only globs level and can miss sharedassets/resS scene data*

qa_roots_in_app globs app/Contents/Resources/Data/level* and scans those bytes for KitRoom. Modern Unity standalone builds also serialize scene hierarchy into sharedassets.assets and .resS split files; on some Unity versions GameObject names for the baked scene live there rather than in level*. If a contaminated build stored the KitRoom_ root name only in sharedassets, this detector returns an empty set and the gate falsely passes the contaminated app. The comment asserts the byte scan is 'reliable' but does not justify why level* is exhaustive. Scan all of Data/.assets and Data/.resS (or at minimum also glob sharedassets*) to close the false-negative gap.

Category: Release regression

Why this matters: The entire value of this PR's sandbox gate is catching the contaminated-build class that drew grey kit masses over every plate; a false-negative path means the regression ships silently, exactly what the gate exists to prevent.

try:
found.update(m.decode() for m in re.findall(rb"KitRoom_[A-Za-z0-9_]+", lvl.read_bytes()))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Helper-name exclusion can mask a real KitRoom_ root (false negative on the contamination gate)

build_room_kit.cs:1113-1127 derives roomId from WORLDOS_KIT_ROOM_ID, the geo location, or the geometry filename. A location/file named e.g. "Fire", "TombGlow", or "CoolKey" produces a REAL root named KitRoom_Fire / KitRoom_TombGlow / KitRoom_CoolKey — exactly the three names this exclusion drops. For those roomIds the gate silently classifies the contaminated root as a helper and qa_roots_in_app returns {} (the comprehension drops it and the or found only triggers when helpers are the SOLE matches; here the real root IS one of those names so it is dropped, and if no other helpers exist the function returns clean). The exclusion should be structural (is the name a known light helper AND present as a Light component), not lexical. At minimum, do not hardcode three names that overlap the roomId namespace; instead detect roots by the absence of a Light component or by matching the KitRoom root convention via the prefab/mass children.

Category: Release regression

Why this matters: The gate's purpose is to never ship a kit root. A roomId collision with a hardcoded helper name is a realistic gap that lets a contaminated build pass the sandbox gate and ship.

except OSError:
continue
# child helper objects (KitRoom_Fire etc.) ride along with a real root; the root name is the signal
return {n for n in found if not n.startswith(("KitRoom_Fire", "KitRoom_TombGlow", "KitRoom_CoolKey"))} or found
Comment on lines +98 to +99

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not re-add filtered child helper names.

Line 99 returns found when every match is a filtered helper, so an otherwise clean app containing only KitRoom_Fire, KitRoom_TombGlow, or KitRoom_CoolKey is still rejected. Return the filtered set directly; a real KitRoom_* root will be present independently. Confidence: 99%.

Proposed fix
-    return {n for n in found if not n.startswith(("KitRoom_Fire", "KitRoom_TombGlow", "KitRoom_CoolKey"))} or found
+    return {n for n in found if not n.startswith(("KitRoom_Fire", "KitRoom_TombGlow", "KitRoom_CoolKey"))}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# child helper objects (KitRoom_Fire etc.) ride along with a real root; the root name is the signal
return {n for n in found if not n.startswith(("KitRoom_Fire", "KitRoom_TombGlow", "KitRoom_CoolKey"))} or found
# child helper objects (KitRoom_Fire etc.) ride along with a real root; the root name is the signal
return {n for n in found if not n.startswith(("KitRoom_Fire", "KitRoom_TombGlow", "KitRoom_CoolKey"))}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qa/qa_sandbox.py` around lines 98 - 99, Update the filtering return
expression in the helper that processes found room names so it always returns
the set excluding names beginning with KitRoom_Fire, KitRoom_TombGlow, or
KitRoom_CoolKey; remove the fallback to found and preserve the existing
root-name filtering behavior.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: or found fallback re-flags helper lights as contamination, blocking legitimate builds

When the only KitRoom_* byte matches in the level file are the surviving helper-light names (KitRoom_Fire/KitRoom_TombGlow/KitRoom_CoolKey), the set comprehension yields {} and the or found clause restores ALL of found, so qa_roots_in_app returns the helper names and the up path at line 143 raises SystemExit('CONTAMINATED'). These helper GameObjects are children of a KitRoom root (build_room_kit.cs:479-500 parents them to root/Lights), so the C# strip already removes them with the root; a level file can also legitimately carry them as orphaned lights that PurgeStrayKitLights left behind (build_room_kit.cs:844-853) without implying a grey-mass root is present. The detector should only fire on a real root: filter found to names NOT in the helper set, and if none remain, return {} (clean) rather than re-flagging the helpers.

Category: Runtime correctness

Why this matters: This is the sandbox contamination gate that blocks the QA lane. A false positive here stops legitimate sweeps with a CONTAMINATED error pointing at names that are not actually roots, and the operator's remediation instruction ('rebuild via BuildMacOSPlayer') will not change anything because there is no root to strip.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: QA-root detector allowlist is a hardcoded prefix tuple that can drift from build_room_kit helper names

qa_roots_in_app excludes KitRoom_Fire, KitRoom_TombGlow, KitRoom_CoolKey via a literal startswith tuple, while those names are defined in build_room_kit.cs (lines 554/561/567). The two lists are maintained in different files/languages with no shared source of truth. If a new KitRoom* nested helper is added in build_room_kit.cs (e.g. a KitRoom_Fog light), the sandbox detector will start flagging clean builds as CONTAMINATED and abort the sandbox up() flow (lines 142-149) — a false-positive CI/sandbox break with no build change. Consider deriving the allowlist from the same place the helper names are authored, or making the detector hierarchy-aware (only flag true root objects) rather than name-based, so it does not need a parallel allowlist at all.

Category: Flaky test risk

Why this matters: A detector that aborts the sandbox on a substring match needs to stay exactly in sync with the C# that emits those names; a drift silently turns clean builds into hard sandbox failures.



def up(run: str, *, campaign: str, engine_port: int, qa_port: int,
seed_cmd: str, app: Path) -> dict:
rd = _rundir(run)
Expand Down Expand Up @@ -119,6 +139,14 @@ def up(run: str, *, campaign: str, engine_port: int, qa_port: int,
if not pbin.exists():
engine.terminate()
raise SystemExit(f"[sandbox] player binary missing: {pbin}")
contaminated = _qa_roots_in_app(app)
if contaminated:
engine.terminate()
raise SystemExit(
f"[sandbox] app CONTAMINATED — QA kit roots baked into the build: {sorted(contaminated)} "
f"({app}). A KitRoom_* scene root was saved into the canonical scene at build time; it "
f"renders grey kit masses over every plate (kit-tavern 2026-07-23). Rebuild via "
f"BuildMacOSPlayer (auto-strips + reports strippedQARoots in build-report.txt).")
Comment on lines +142 to +149

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Run the contamination gate before provisioning the engine.

The code path starts and waits for the engine at Lines 127-134 before checking contamination. A rejected app therefore still seeds state and launches an engine; terminate() is non-blocking and no run metadata exists to clean up a lingering process. Move the binary and contamination preflight to the start of up(), before creating the run directory or spawning subprocesses. Confidence: 98%.

🧰 Tools
🪛 Ruff (0.15.21)

[warning] 145-149: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qa/qa_sandbox.py` around lines 142 - 149, Move the binary validation and
_qa_roots_in_app contamination check to the beginning of up(), before creating
run metadata, provisioning the engine, or spawning subprocesses. Reject
contaminated or invalid apps immediately, preserving the existing contamination
error details while removing the need to terminate an engine that has not
started.

penv = dict(os.environ,
WORLDOS_ENGINE_BASE_URL=f"http://127.0.0.1:{engine_port}",
WORLDOS_CAMPAIGN_ID=campaign,
Expand Down
Loading