Skip to content

build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan #1690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
100yenadmin wants to merge 4 commits into
base: main
Choose a base branch
from
Open

build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan #1690

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0dedfa2
build/qa: kit-scene contamination gate — auto-strip KitRoom_* at buil…
100yenadmin Jul 23, 2026
3e0104f
renderer: kit-derived occluder sidecar (ExportBoxes) + tavern swap — …
100yenadmin Jul 23, 2026
ecfa2e1
qa+plates: per-object alignment gate (red-first: caught c2 deleting t…
100yenadmin Jul 23, 2026
607a165
plates: crypt kit-derived sidecar (73 true-bounds boxes) — same felt-…
100yenadmin Jul 23, 2026
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
64 changes: 64 additions & 0 deletions extensions/renderers/unity/boxes/tavern_kit_v1_boxes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
{
"version": 1,
"ortho": 10.5224,
"cols": 14, "rows": 11,
"provenance": "kit-derived (build_room_kit ExportBoxes): per-mass renderer bounds of KitRoom_tavern",
"boxes": [
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-13, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-11, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-9, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-7, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-5, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-3, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [-1, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [3, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [5, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [7, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [9, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [11, 0, 10]},
{"kind": "wallback", "size": [2, 5.4, 1.4], "center": [13, 0, 10]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-11, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-9, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-7, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-5, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-3, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [-1, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [1, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [3, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [5, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [7, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [9, 0, -10]},
{"kind": "parapet", "size": [2, 0.55, 1.4], "center": [11, 0, -10]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, -10]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, 8]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, 6]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, 4]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, 2]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, 0]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, -2]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, -4]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, -6]},
{"kind": "parapet", "size": [1.4, 0.55, 2], "center": [-13, 0, -8]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, 8]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, 6]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, 4]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, 2]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, -2]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, -4]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, -6]},
{"kind": "wallright", "size": [1.4, 5.4, 2], "center": [13, 0, -8]},
{"kind": "bar", "size": [7.2, 1.4, 1.8], "center": [-6, 0.7, 6]},
{"kind": "barrel", "size": [1.8, 1.5, 1.8], "center": [-9, 0.75, 8]},
{"kind": "barrel", "size": [1.8, 1.5, 1.8], "center": [-7, 0.75, 8]},
{"kind": "hearth", "size": [3.6, 1.4, 1.8], "center": [8, 0.7, 8]},
{"kind": "table", "size": [1.8, 1.4, 3.6], "center": [-5, 0.7, -1]},
{"kind": "table", "size": [1.8, 1.4, 3.6], "center": [5, 0.7, 1]},
{"kind": "table", "size": [3.6, 1.4, 1.8], "center": [2, 0.7, -4]},
{"kind": "table", "size": [3.6, 1.4, 1.8], "center": [10, 0.7, -4]},
{"kind": "candle", "size": [0.95, 1.95, 0.95], "center": [3, 0.975, 8]},
{"kind": "candle", "size": [1.3, 0.45, 1.3], "center": [3, 0.225, 8]},
{"kind": "candle", "size": [0.95, 1.95, 0.95], "center": [11, 0.975, 4]},
{"kind": "candle", "size": [1.3, 0.45, 1.3], "center": [11, 0.225, 4]}
]
}
4 changes: 2 additions & 2 deletions extensions/renderers/unity/plates_manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@
"pitch": 30,
"yaw": 45
},
"boxes": "boxes/tavern_v2_boxes.json",
"_provenance": "KIT-TAVERN v1 (2026-07-23, #83 replication proof): SAME certified chain as KIT-CRYPT v1 \u2014 geometry tavern_v2 -> kit scene (seg gate 100.00% FIRST TRY, 0/154) -> kit 3D depth -> flux base seed 12345 (recall 0.9737+, asset_G5UBNVwMchoG84wPSFiBEsAZ) -> Gemini 3.0 Pro edit c2 w/ FURNITURE-IDENTITY grounding (job_UTwT9WJkbvZpuveXDoMso8Eh) -> phase-corr ALIGNED dx=0 -> seg-mask void composite. Blind panel 7 vs control 8 (delta -1.0 in-band; c1 without furniture grounding was 6/-2.0 \u2014 the grounding clause is load-bearing). Collision truth UNCHANGED (tavern_v2 geometry/walkmask/boxes)."
"boxes": "boxes/tavern_kit_v1_boxes.json",
"_provenance": "KIT-TAVERN v1 (2026-07-23, #83 replication proof): SAME certified chain as KIT-CRYPT v1 \u2014 geometry tavern_v2 -> kit scene (seg gate 100.00% FIRST TRY, 0/154) -> kit 3D depth -> flux base seed 12345 (recall 0.9737+, asset_G5UBNVwMchoG84wPSFiBEsAZ) -> Gemini 3.0 Pro edit c2 w/ FURNITURE-IDENTITY grounding (job_UTwT9WJkbvZpuveXDoMso8Eh) -> phase-corr ALIGNED dx=0 -> seg-mask void composite. Blind panel 7 vs control 8 (delta -1.0 in-band; c1 without furniture grounding was 6/-2.0 \u2014 the grounding clause is load-bearing). Collision truth UNCHANGED (tavern_v2 geometry/walkmask/boxes). SIDECAR 2026-07-23: kit-derived boxes (ExportBoxes per-mass true bounds; cutaway parapets 0.55u, tables 1.4u true footprints) replacing the authored tavern_v2 chunky volumes \u2014 felt-truth fix for the owner-caught silhouette-everywhere class."

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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: Provenance claims 'Collision truth UNCHANGED (tavern_v2 geometry/walkmask/boxes)' while the boxes entry is swapped

The _provenance string still asserts 'Collision truth UNCHANGED (tavern_v2 ... boxes)' but the immediately preceding line swaps boxes from boxes/tavern_v2_boxes.json to boxes/tavern_kit_v1_boxes.json. If the runtime consumes these boxes for anything beyond silhouette occluder proxies (e.g., any movement/collision hint), the statement is now self-contradictory. Clarify in the provenance that the swap affects occluder/silhouette sidecars only and that engine collision still reads tavern_v2 geometry/walkmask — or confirm no runtime path reads boxes for collision and note it.

Category: API compatibility

Why this matters: Self-contradictory provenance on a certified plate undermines the release-evidence trail the RRI contract depends on; a reviewer cannot tell whether collision behavior changed.

},
"throne_hall": {
"plate": "plates/throne_hall_v1_registered.png",
Expand Down
35 changes: 35 additions & 0 deletions extensions/renderers/unity/scripts/BuildMacOSPlayer.cs
View file
Open in desktop
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);

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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
72 changes: 72 additions & 0 deletions extensions/renderers/unity/scripts/build_room_kit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,78 @@ static string CaptureDir() =>
// ════════════════════════════════════════════════════════════════════════════════════════════════
// BUILD
// ════════════════════════════════════════════════════════════════════════════════════════════════
// Occluder-sidecar export DERIVED from the built kit scene: per-mass true renderer bounds instead of
// hand-authored chunky volumes. The felt-truth failure this kills (owner playtest 2026-07-23): the
// authored tavern sidecar carried 5.0u camera-side walls where the paint shows 0.55u cutaway parapets
// and square slabs around round tables, so the walk-behind silhouette fired far outside every painted
// object and legal moves read as walking through furniture. The kit scene IS the plate's geometry
// (seg-gate + alignment certified), so bounds measured off the placed objects match the paint by
// construction. Requires BuildRoom to have run for the same geometry this editor session.
[MenuItem("Tools/WorldOS/Kit/Export Kit Boxes")]
public static void ExportBoxes()
{
string geoPath = GeoPath();
if (!File.Exists(geoPath)) { Debug.LogError($"[KitBoxes] no geometry json: {geoPath}"); return; }
var geo = MiniJson.Parse(File.ReadAllText(geoPath)) as Dictionary<string, object>;
if (geo == null) { Debug.LogError("[KitBoxes] geometry parse failed"); return; }
int cols = GetInt(geo, "cols", 14), rows = GetInt(geo, "rows", 11);
bool camFit = GetBool(geo, "camera_fit");
string roomId = RoomId(geo, geoPath);

var root = GameObject.Find("KitRoom_" + roomId);
if (root == null) { Debug.LogError($"[KitBoxes] KitRoom_{roomId} not in scene — run Build Room From Kit first."); return; }

// the CONTRACT ortho (same math as SetupContractCamera at the 1344x768 contract aspect)
float aspect = 1344f / 768f, FILL = 0.96f, ortho = 13f;
if (camFit)
{
Quaternion crot = Quaternion.Euler(30f, 45f, 0f);
Vector3 rightAx = crot * Vector3.right, upAx = crot * Vector3.up;
float maxR = 0f, maxU = 0f, hx = (cols / 2f) * CELL, hz = (rows / 2f) * CELL;
foreach (var sgn in new[] { new Vector2(1, 1), new Vector2(1, -1), new Vector2(-1, 1), new Vector2(-1, -1) })
{
Vector3 corner = new Vector3(hx * sgn.x, 0f, hz * sgn.y);
maxR = Mathf.Max(maxR, Mathf.Abs(Vector3.Dot(corner, rightAx)));
maxU = Mathf.Max(maxU, Mathf.Abs(Vector3.Dot(corner, upAx)));
}
ortho = Mathf.Max(maxR / (aspect * FILL), maxU / FILL);
}

var inv = System.Globalization.CultureInfo.InvariantCulture;
var sb = new System.Text.StringBuilder();
sb.Append("{\n \"version\": 1,\n");
sb.Append(string.Format(inv, " \"ortho\": {0:0.####},\n", ortho));
sb.Append(string.Format(inv, " \"cols\": {0}, \"rows\": {1},\n", cols, rows));
sb.Append(" \"provenance\": \"kit-derived (build_room_kit ExportBoxes): per-mass renderer bounds of KitRoom_" + roomId + "\",\n");
sb.Append(" \"boxes\": [\n");
int nBoxes = 0;
foreach (var groupName in new[] { "Walls", "Props", "Impassable" })
{
var group = root.transform.Find(groupName);
if (group == null) continue;
foreach (Transform child in group)
{
var rends = child.GetComponentsInChildren<Renderer>(false);
if (rends.Length == 0) continue;
Bounds b = rends[0].bounds;
for (int i = 1; i < rends.Length; i++) b.Encapsulate(rends[i].bounds);
if (b.size.y < 0.15f) continue; // flat decals never occlude
string kind = child.name.ToLowerInvariant();
int cut = kind.IndexOf('_'); if (cut > 0) kind = kind.Substring(0, cut);
if (nBoxes > 0) sb.Append(",\n");
sb.Append(string.Format(inv,
" {{\"kind\": \"{0}\", \"size\": [{1:0.###}, {2:0.###}, {3:0.###}], \"center\": [{4:0.###}, {5:0.###}, {6:0.###}]}}",
kind, b.size.x, b.size.y, b.size.z, b.center.x, b.center.y, b.center.z));
nBoxes++;
}
}
sb.Append("\n ]\n}\n");
string projectRoot = Directory.GetParent(Application.dataPath).FullName;
string outPath = Path.Combine(projectRoot, "room_kit_boxes.json");

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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: ExportBoxes writes room_kit_boxes.json to project root, not the boxes/ dir the build packages

ExportBoxes writes its output to Path.Combine(projectRoot, "room_kit_boxes.json") — the project root, not /boxes/. The player packaging step (BuildMacOSPlayer.EnsurePackaged lines 98-104) copies boxes/*.json from /boxes/ into StreamingAssets, and plates_manifest.json references boxes/tavern_kit_v1_boxes.json. The exported file at project root is never picked up by packaging and has the wrong filename; a developer following the menu path produces a sidecar that silently does not ship. Write directly to /boxes/_kit_v1_boxes.json (matching the manifest reference and the packaging glob) so the export→ship chain is contiguous.

Category: Unity scene/prefab

Why this matters: The PR ships a tavern_kit_v1_boxes.json that the ExportBoxes menu item cannot produce at the path the build consumes, leaving the certified chain non-reproducible from the tooling alone.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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: ExportBoxes writes room_kit_boxes.json to project root, not the boxes/ dir the manifest consumes

ExportBoxes() writes its output to Path.Combine(projectRoot, "room_kit_boxes.json") (project root), but plates_manifest.json references boxes/crypt_kit_v1_boxes.json and boxes/tavern_kit_v1_boxes.json, and the committed artifacts under extensions/renderers/unity/boxes/ are named per-room (crypt_kit_v1_boxes.json / tavern_kit_v1_boxes.json). There is no automated copy/rename step and no CI gate tying the ExportBoxes output to the committed sidecars, so the menu-produced file and the manifest-consumed artifacts can silently drift — a future re-export could be missed or hand-edited divergently. The provenance strings in the committed JSON claim "kit-derived (build_room_kit ExportBoxes)" but nothing enforces that the committed file equals an ExportBoxes run for the current geometry. Consider writing directly into boxes/_kit_v1_boxes.json (parametrized by roomId) or adding a deterministic copy step, plus a lint that re-derives and diffs.

Category: Unity scene/prefab

Why this matters: The occluder sidecar drives the walk-behind silhouette; if the committed boxes drift from what ExportBoxes produces for the current geometry, the felt-truth fix this PR ships can silently regress on the next kit rebuild with no signal.

File.WriteAllText(outPath, sb.ToString());
Debug.Log($"[KitBoxes] wrote {nBoxes} kit-derived boxes (ortho {ortho:0.####}) -> {outPath}");
}

[MenuItem("Tools/WorldOS/Kit/Build Room From Kit")]
public static void BuildRoom()
{
Expand Down
28 changes: 28 additions & 0 deletions qa/qa_sandbox.py
View file
Open in desktop
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*")):

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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

@coderabbitai coderabbitai Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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.

@evaos-code-review-bot evaos-code-review-bot Bot Jul 23, 2026

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

@coderabbitai coderabbitai Bot Jul 23, 2026

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