build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan#1690
build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan#1690100yenadmin wants to merge 4 commits into
Conversation
…d + sandbox level-scan (#85 grey-tops root cause)
📝 WalkthroughPriority Level: P1
WalkthroughThe Unity pipeline exports kit bounds, registers crypt and tavern renderer datasets, validates per-object image alignment, strips QA roots from macOS builds, and rejects contaminated sandbox applications. Public build entry points remain unchanged. Confidence: 95% ChangesUnity kit rendering and QA validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UnityKitExporter
participant KitRoomScene
participant RendererManifest
participant AlignmentChecker
participant BaseRender
participant StyledRender
UnityKitExporter->>KitRoomScene: measure KitRoom geometry
UnityKitExporter->>RendererManifest: provide kit box dataset
AlignmentChecker->>RendererManifest: load boxes and ortho
AlignmentChecker->>BaseRender: crop projected object regions
AlignmentChecker->>StyledRender: crop matching regions
AlignmentChecker->>AlignmentChecker: phase-correlate and enforce cell budget
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)extensions/renderers/unity/boxes/crypt_kit_v1_boxes.jsonTraceback (most recent call last): extensions/renderers/unity/boxes/tavern_kit_v1_boxes.jsonTraceback (most recent call last): extensions/renderers/unity/plates_manifest.jsonTraceback (most recent call last): Comment |
evaOS review status: completedPR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1690 Review URL: #1690 (review) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@qa/qa_sandbox.py`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42d7f6d8-a45b-4acb-8416-00b79768998d
📒 Files selected for processing (2)
extensions/renderers/unity/scripts/BuildMacOSPlayer.csqa/qa_sandbox.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
- GitHub Check: qa-release-gate-tests
- GitHub Check: viewer-tests
- GitHub Check: test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (csharp)
- GitHub Check: test
- GitHub Check: viewer-tests
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat/Users/lume/WorldOSas the canonical local Mac app/private-art checkout for WorldOS GUI and native-app testing.
Use/Volumes/LEXAR/Codexfor Codex artifacts, scratch files, screenshots, reports, and downloaded CI/VM artifacts.
Before install, build, or test commands, verifypwd; GUI/native app runs must use/Users/lume/WorldOSor an approved same-disk worktree configuration.
Use thecodex/prefix for new branches unless instructed otherwise.
Files:
qa/qa_sandbox.pyextensions/renderers/unity/scripts/BuildMacOSPlayer.cs
🪛 Ruff (0.15.21)
qa/qa_sandbox.py
[warning] 96-97: try-except within a loop incurs performance overhead
(PERF203)
[warning] 145-149: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs (1)
191-194: LGTM!Also applies to: 266-303
| # 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 |
There was a problem hiding this comment.
🎯 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.
| # 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.
| 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).") |
There was a problem hiding this comment.
🩺 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.
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan
Head: 0dedfa29440ed865fe45a642e384b007a482fd44 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 3/5 (~34 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +35/-0 | Unity/gameplay state | Elevated: validated P1 finding |
qa/qa_sandbox.py |
modified | +28/-0 | Changed file | Elevated: validated P1 finding |
Review Signal
Validated inline findings: 6 (P0: 0, P1: 2, P2: 2, P3: 2).
Dropped findings before posting: 0. High-severity findings: 2.
Risk Taxonomy
- Data loss: 1
- Flaky test risk: 1
- Release regression: 2
- Runtime correctness: 1
- Unity scene/prefab: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #85, #83.
Suggested labels: bug, unity.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| } | ||
| if (stripped.Count > 0) | ||
| { | ||
| UnityEditor.SceneManagement.EditorSceneManager.SaveScene(scene); |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| data_dir = app / "Contents" / "Resources" / "Data" | ||
| for lvl in sorted(data_dir.glob("level*")): | ||
| try: | ||
| found.update(m.decode() for m in re.findall(rb"KitRoom_[A-Za-z0-9_]+", lvl.read_bytes())) |
There was a problem hiding this comment.
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.
|
|
||
| static string[] StripQAConstructions() | ||
| { | ||
| var scene = UnityEditor.SceneManagement.EditorSceneManager.OpenScene(SceneToBuild); |
There was a problem hiding this comment.
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.
| } | ||
| if (stripped.Count > 0) | ||
| { | ||
| UnityEditor.SceneManagement.EditorSceneManager.SaveScene(scene); |
There was a problem hiding this comment.
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.
| import re | ||
| found: set = set() | ||
| data_dir = app / "Contents" / "Resources" / "Data" | ||
| for lvl in sorted(data_dir.glob("level*")): |
There was a problem hiding this comment.
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.
…felt-truth fix for owner-caught silhouette-everywhere (parapets 0.55u vs authored 5.0u walls, true table footprints)
evaOS review status: completedPR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1690 Review URL: #1690 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan
Head: 3e0104f96e524a6da08473d3da9480bceb5f729e into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 2/5 (~30 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/boxes/tavern_kit_v1_boxes.json |
added | +64/-0 | Changed file | Low |
extensions/renderers/unity/plates_manifest.json |
modified | +2/-2 | Changed file | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +35/-0 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/build_room_kit.cs |
modified | +72/-0 | Unity/gameplay state | Moderate: validated P3 finding |
qa/qa_sandbox.py |
modified | +28/-0 | Changed file | Moderate: validated P2 finding |
Review Signal
Validated inline findings: 5 (P0: 0, P1: 0, P2: 3, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Release regression: 2
- Runtime correctness: 1
- Unity scene/prefab: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #85, #83.
Suggested labels: unity.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
|
|
||
| static string[] StripQAConstructions() | ||
| { | ||
| var scene = UnityEditor.SceneManagement.EditorSceneManager.OpenScene(SceneToBuild); |
There was a problem hiding this comment.
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).
| } | ||
| if (stripped.Count > 0) | ||
| { | ||
| UnityEditor.SceneManagement.EditorSceneManager.SaveScene(scene); |
There was a problem hiding this comment.
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.
| import re | ||
| found: set = set() | ||
| data_dir = app / "Contents" / "Resources" / "Data" | ||
| for lvl in sorted(data_dir.glob("level*")): |
There was a problem hiding this comment.
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.
| } | ||
| sb.Append("\n ]\n}\n"); | ||
| string projectRoot = Directory.GetParent(Application.dataPath).FullName; | ||
| string outPath = Path.Combine(projectRoot, "room_kit_boxes.json"); |
There was a problem hiding this comment.
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.
| "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." |
There was a problem hiding this comment.
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.
…able#51) + tavern v2 plate (surgical gemini re-add -> diff-mask composite, PER-OBJECT-ALIGNED 12/12)
…truth treatment as tavern
evaOS review status: completedPR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1690 Review URL: #1690 (review) |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@extensions/renderers/unity/plates_manifest.json`:
- Line 52: The provenance entries at
extensions/renderers/unity/plates_manifest.json lines 52-52 and 82-82
incorrectly claim the collision boxes are unchanged. Update both entries to
state that engine geometry and walkmask collision remain unchanged while the
visual occluder sidecar now uses new kit-derived boxes; preserve the existing
provenance context.
In `@qa/object_align_check.py`:
- Around line 83-100: Update the object-check loop around box_screen_bbox and
the checked/failures counters so undersized eligible non-wall boxes cannot be
silently skipped. Mark them as failures unless the sidecar explicitly exempts
them, and report any explicit exemptions separately; ensure an all-uncheckable
set cannot produce PER-OBJECT-ALIGNED with exit 0.
- Around line 40-52: Update local_phasecorr and its callers to reject
low-variance inputs and correlation responses below a threshold calibrated from
known aligned fixtures, returning the established UNCHECKABLE failure state
rather than (0, 0, 0). Ensure the validation path around the result handling
near the referenced lines does not convert rejected featureless crops into
err_cells=0 or a passing gate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21ce1f6c-2e11-4d3f-a2f8-5362f4da2108
⛔ Files ignored due to path filters (1)
extensions/renderers/unity/plates/tavern_kit_v2_registered.pngis excluded by!**/*.png
📒 Files selected for processing (5)
extensions/renderers/unity/boxes/crypt_kit_v1_boxes.jsonextensions/renderers/unity/boxes/tavern_kit_v1_boxes.jsonextensions/renderers/unity/plates_manifest.jsonextensions/renderers/unity/scripts/build_room_kit.csqa/object_align_check.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: viewer-tests
- GitHub Check: test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: viewer-tests
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat/Users/lume/WorldOSas the canonical local Mac app/private-art checkout for WorldOS GUI and native-app testing.
Use/Volumes/LEXAR/Codexfor Codex artifacts, scratch files, screenshots, reports, and downloaded CI/VM artifacts.
Before install, build, or test commands, verifypwd; GUI/native app runs must use/Users/lume/WorldOSor an approved same-disk worktree configuration.
Use thecodex/prefix for new branches unless instructed otherwise.
Files:
extensions/renderers/unity/boxes/tavern_kit_v1_boxes.jsonextensions/renderers/unity/boxes/crypt_kit_v1_boxes.jsonqa/object_align_check.pyextensions/renderers/unity/plates_manifest.jsonextensions/renderers/unity/scripts/build_room_kit.cs
🧠 Learnings (1)
📚 Learning: 2026-07-16T03:59:05.631Z
Learnt from: 100yenadmin
Repo: electricsheephq/WorldOS PR: 1602
File: extensions/renderers/unity/boxes/tavern_snug_v1_boxes.json:42-55
Timestamp: 2026-07-16T03:59:05.631Z
Learning: For WorldOS Unity renderer sidecar box JSON files under extensions/renderers/unity/boxes/*_boxes.json, treat boxes labeled/typed `wall_run` and `door_frame` as occlusion-depth proxies only. Do not flag them as traversal/collision obstructions based solely on visible overlap with the wall-run/jamb doorway reveal geometry. Traversal and collision should be determined only by the engine-side walkmask/room geometry and the actual engine collision path; only raise an obstruction issue if there is evidence that the walkmask or engine collision indicates a blocked traversal.
Applied to files:
extensions/renderers/unity/boxes/tavern_kit_v1_boxes.jsonextensions/renderers/unity/boxes/crypt_kit_v1_boxes.json
🪛 OpenGrep (1.25.0)
extensions/renderers/unity/scripts/build_room_kit.cs
[WARNING] 150-150: File operation with dynamic path can lead to path traversal. Validate and sanitize file paths against a safe base directory.
(coderabbit.path-traversal.csharp-file-read)
🪛 Ruff (0.15.21)
qa/object_align_check.py
[error] 41-41: Multiple statements on one line (semicolon)
(E702)
[error] 43-43: Multiple statements on one line (colon)
(E701)
[error] 44-44: Multiple statements on one line (colon)
(E701)
[error] 45-45: Multiple statements on one line (semicolon)
(E702)
[error] 46-46: Multiple statements on one line (semicolon)
(E702)
[error] 47-47: Multiple statements on one line (semicolon)
(E702)
[error] 50-50: Multiple statements on one line (colon)
(E701)
[error] 51-51: Multiple statements on one line (colon)
(E701)
[error] 56-56: Multiple statements on one line (semicolon)
(E702)
[error] 62-62: Multiple statements on one line (semicolon)
(E702)
[error] 63-63: Multiple statements on one line (semicolon)
(E702)
[error] 64-64: Multiple statements on one line (semicolon)
(E702)
[error] 70-70: Multiple statements on one line (semicolon)
(E702)
[error] 70-70: Multiple statements on one line (semicolon)
(E702)
🔇 Additional comments (1)
extensions/renderers/unity/scripts/build_room_kit.cs (1)
204-206: Existing packaging-path defect remains.ExportBoxes()writes<projectRoot>/room_kit_boxes.json, but the manifest consumesboxes/crypt_kit_v1_boxes.json/boxes/tavern_kit_v1_boxes.json. Re-running the menu action therefore cannot regenerate the packaged sidecars. Write toboxes/${roomId}_kit_v1_boxes.jsoninstead. Confidence: 100%.
| ], | ||
| "_effects_comment": "Animated fire over the four painted brazier fires (geometry brazier cells; bowls sit ~1.2u up the stem). Tune scale/y at the sandbox eyeball if the flame floats or washes the paint.", | ||
| "_provenance": "KIT-CRYPT v1 (2026-07-23, #83/#84 nav-truth spike): 3D-FIRST CERTIFIED CHAIN \u2014 geometry crypt_v36 -> build_room_kit scene (footprint SEG GATE 100.00%, 0/192 disagreements, qa/evidence/kit-crypt/) -> kit 3D depth -> flux depth-CN base seed 12346 (recall 0.9752, asset_qM9R3tuRH4sceUrVjCoLdPRk) -> Gemini 3.0 Pro edit (model_google-gemini-pro-image-editing, critique-targeted detail prompt, job_Lmhssjh7iWnHsUKY1dhnHB4m) -> phase-corr ALIGNED dx=0,dy=0 vs base (qa/styled_align_check.py) -> geometric void composite (seg-frame mask). Blind panel 7 vs control 8 (delta -1.0, in-band; wf_89967920). Collision truth UNCHANGED (same geometry/walkmask/boxes as crypt_v36). NOTE: model_google-gemini-pro-image-editing pending model-registry allowlist (owner sign-off)." | ||
| "_provenance": "KIT-CRYPT v1 (2026-07-23, #83/#84 nav-truth spike): 3D-FIRST CERTIFIED CHAIN \u2014 geometry crypt_v36 -> build_room_kit scene (footprint SEG GATE 100.00%, 0/192 disagreements, qa/evidence/kit-crypt/) -> kit 3D depth -> flux depth-CN base seed 12346 (recall 0.9752, asset_qM9R3tuRH4sceUrVjCoLdPRk) -> Gemini 3.0 Pro edit (model_google-gemini-pro-image-editing, critique-targeted detail prompt, job_Lmhssjh7iWnHsUKY1dhnHB4m) -> phase-corr ALIGNED dx=0,dy=0 vs base (qa/styled_align_check.py) -> geometric void composite (seg-frame mask). Blind panel 7 vs control 8 (delta -1.0, in-band; wf_89967920). Collision truth UNCHANGED (same geometry/walkmask/boxes as crypt_v36). NOTE: model_google-gemini-pro-image-editing pending model-registry allowlist (owner sign-off). SIDECAR 2026-07-23: kit-derived boxes (ExportBoxes, 73 per-mass true bounds: parapets 0.55u, pillars/tomb/buttresses at real heights) replacing the authored crypt_v36 volumes -- silhouette bands now hug the paint." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the stale “boxes unchanged” provenance. Both entries replace the occluder sidecar, so claiming the prior boxes are unchanged makes the release evidence self-contradictory. Preserve that engine geometry/walkmask collision is unchanged, but explicitly state that the visual occluder sidecar changed. Confidence: 100%.
extensions/renderers/unity/plates_manifest.json#L52-L52: replacesame geometry/walkmask/boxes as crypt_v36with wording that distinguishes unchanged geometry/walkmask from the new kit-derived boxes.extensions/renderers/unity/plates_manifest.json#L82-L82: replacetavern_v2 geometry/walkmask/boxeswith the same distinction.
📍 Affects 1 file
extensions/renderers/unity/plates_manifest.json#L52-L52(this comment)extensions/renderers/unity/plates_manifest.json#L82-L82
🤖 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 `@extensions/renderers/unity/plates_manifest.json` at line 52, The provenance
entries at extensions/renderers/unity/plates_manifest.json lines 52-52 and 82-82
incorrectly claim the collision boxes are unchanged. Update both entries to
state that engine geometry and walkmask collision remain unchanged while the
visual occluder sidecar now uses new kit-derived boxes; preserve the existing
provenance context.
| def local_phasecorr(a, b): | ||
| a = a - a.mean(); b = b - b.mean() | ||
| sa, sb = a.std(), b.std() | ||
| if sa > 0: a = a / sa | ||
| if sb > 0: b = b / sb | ||
| wy = np.hanning(a.shape[0])[:, None]; wx = np.hanning(a.shape[1])[None, :] | ||
| A = np.fft.rfft2(a * wy * wx); B = np.fft.rfft2(b * wy * wx) | ||
| R = A * np.conj(B); R /= np.abs(R) + 1e-9 | ||
| r = np.fft.irfft2(R, a.shape) | ||
| dy, dx = np.unravel_index(np.argmax(r), r.shape) | ||
| if dy > a.shape[0] // 2: dy -= a.shape[0] | ||
| if dx > a.shape[1] // 2: dx -= a.shape[1] | ||
| return int(dx), int(dy), float(r.max()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject featureless correlation results instead of passing them as zero drift (Confidence: 99%).
For constant/blank crops, mean-centering makes both spectra zero; argmax then returns (0, 0) with resp=0. Line 90 returns that result and Line 92 passes it as err_cells=0. Missing, fully occluded, or unrendered objects can therefore produce a green gate. Treat low-variance and low-response results as UNCHECKABLE failures, with a response threshold calibrated from known aligned fixtures.
Also applies to: 90-92
🧰 Tools
🪛 Ruff (0.15.21)
[error] 41-41: Multiple statements on one line (semicolon)
(E702)
[error] 43-43: Multiple statements on one line (colon)
(E701)
[error] 44-44: Multiple statements on one line (colon)
(E701)
[error] 45-45: Multiple statements on one line (semicolon)
(E702)
[error] 46-46: Multiple statements on one line (semicolon)
(E702)
[error] 47-47: Multiple statements on one line (semicolon)
(E702)
[error] 50-50: Multiple statements on one line (colon)
(E701)
[error] 51-51: Multiple statements on one line (colon)
(E701)
🤖 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/object_align_check.py` around lines 40 - 52, Update local_phasecorr and
its callers to reject low-variance inputs and correlation responses below a
threshold calibrated from known aligned fixtures, returning the established
UNCHECKABLE failure state rather than (0, 0, 0). Ensure the validation path
around the result handling near the referenced lines does not convert rejected
featureless crops into err_cells=0 or a passing gate.
| for i, box in enumerate(sidecar["boxes"]): | ||
| kind = str(box.get("kind", "?")).lower() | ||
| if kind in SKIP_KINDS: | ||
| continue | ||
| x0, y0, x1, y1 = box_screen_bbox(box, ortho, args.pad) | ||
| if x1 - x0 < 48 or y1 - y0 < 48: | ||
| continue # too small for a stable local spectrum | ||
| dx, dy, resp = local_phasecorr(styled[y0:y1, x0:x1], base[y0:y1, x0:x1]) | ||
| err_cells = math.hypot(dx, dy) / px_per_cell | ||
| ok = err_cells <= args.budget_cells | ||
| checked += 1 | ||
| if not ok: | ||
| failures += 1 | ||
| print(f" {kind}#{i} @({box['center'][0]:g},{box['center'][2]:g}) " | ||
| f"dx={dx} dy={dy} resp={resp:.3f} err_cells={err_cells:.2f} -> {'OK' if ok else 'DRIFTED'}") | ||
| verdict = "PER-OBJECT-ALIGNED" if failures == 0 else f"OBJECT-DRIFT ({failures}/{checked} out of budget)" | ||
| print(f"{verdict} budget={args.budget_cells} cells, px/cell={px_per_cell:.1f}") | ||
| sys.exit(0 if failures == 0 else 1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently exempt non-wall objects from the gate (Confidence: 98%).
Line 89 skips undersized projected boxes before either counter changes. If every eligible box is small, Line 98 prints PER-OBJECT-ALIGNED and exits 0 despite checking nothing. Fail uncheckable non-wall boxes, or require an explicit sidecar exemption and report it separately.
🤖 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/object_align_check.py` around lines 83 - 100, Update the object-check loop
around box_screen_bbox and the checked/failures counters so undersized eligible
non-wall boxes cannot be silently skipped. Mark them as failures unless the
sidecar explicitly exempts them, and report any explicit exemptions separately;
ensure an all-uncheckable set cannot produce PER-OBJECT-ALIGNED with exit 0.
There was a problem hiding this comment.
Walkthrough
PR: #1690 - build/qa: kit-scene contamination gate — auto-strip KitRoom_* roots at build + sandbox level-file scan
Head: 607a165572d0600c8320ecda3672b28a4531e42e into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 4/5 (~56 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/boxes/crypt_kit_v1_boxes.json |
added | +81/-0 | Changed file | Low |
extensions/renderers/unity/boxes/tavern_kit_v1_boxes.json |
added | +64/-0 | Changed file | Low |
extensions/renderers/unity/plates/tavern_kit_v2_registered.png |
added | +0/-0 | Changed file | Low |
extensions/renderers/unity/plates_manifest.json |
modified | +5/-5 | Changed file | Low |
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +35/-0 | Unity/gameplay state | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/build_room_kit.cs |
modified | +72/-0 | Unity/gameplay state | Moderate: validated P3 finding |
qa/object_align_check.py |
added | +104/-0 | Changed file | Moderate: validated P3 finding |
qa/qa_sandbox.py |
modified | +28/-0 | Changed file | Moderate: validated P3 finding |
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 0, P3: 4).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Data loss: 1
- Flaky test risk: 1
- Release regression: 1
- Unity scene/prefab: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #85, #83.
Suggested labels: unity.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| } | ||
| if (stripped.Count > 0) | ||
| { | ||
| UnityEditor.SceneManagement.EditorSceneManager.SaveScene(scene); |
There was a problem hiding this comment.
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.
| } | ||
| sb.Append("\n ]\n}\n"); | ||
| string projectRoot = Directory.GetParent(Application.dataPath).FullName; | ||
| string outPath = Path.Combine(projectRoot, "room_kit_boxes.json"); |
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
P3: Per-object alignment gate is not wired to CI and ships no committed evidence
object_align_check.py is added as a manual instrument (sys.exit 0/1) but nothing in this diff invokes it from CI, a pre-merge gate, or a makefile target, and no evidence file (e.g. qa/evidence/...) is committed alongside it. The tavern _provenance in plates_manifest.json asserts "PER-OBJECT-ALIGNED 12/12 (table#51 0.13 cells)" as load-bearing proof for swapping the plate and boxes, but that claim is only reproducible by a human re-running the script by hand with the right base/styled PNGs. If the gate is intended to be a real release-readiness signal, it needs deterministic wiring (inputs pinned, threshold fixed at --budget-cells 0.35) and captured output; otherwise the claim is unenforced and the table#51 regression class could return undetected.
Category: Release regression
Why this matters: The PR's whole premise is that global phase-corr missed a per-object drift (table#51 deletion); shipping the detector but not enforcing it leaves the exact regression class it was built to catch unguarded.
| 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 |
There was a problem hiding this comment.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607a165572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue # too small for a stable local spectrum | ||
| dx, dy, resp = local_phasecorr(styled[y0:y1, x0:x1], base[y0:y1, x0:x1]) | ||
| err_cells = math.hypot(dx, dy) / px_per_cell | ||
| ok = err_cells <= args.budget_cells |
There was a problem hiding this comment.
When a styled crop has no usable signal (for example, the style pass paints a checked table/hearth into a flat floor patch, or the wrong base/styled image is supplied), local_phasecorr can return (dx, dy) = (0, 0) with resp near zero because there is no meaningful correlation peak, but this line treats it as OK solely by offset. That lets the new per-object gate falsely certify missing or deleted objects—the exact result-corruption class this check is supposed to catch—so the pass condition should also fail on low response/variance rather than only err_cells.
Useful? React with 👍 / 👎.
1 participant
Root cause (the withheld tavern install)
The grey slabs over the new tavern plate were the kit-tavern QA scene embedded in the player build — `strings level0` of the sandbox app shows `KitRoom_tavern` + `table_a-d_fallback`, `bar_counter_fallback`, `hearth_fallback`, `candle_door/hearth_Brazier(+Base)`, `barrel_*_fallback`. A capture flow saved the canonical scene while the kit root existed; the build shipped it; the kit fallback boxes/plinths/parapets then render in front of the (clean, byte-identical) plate. Third occurrence of this class (kit-crypt cleankit trap ×2).
Killed hypotheses, each by measurement: plate/composite defect (plate clean + sha-identical repo↔app), hot-load manifest seam (entries structurally equivalent), visible occluder proxies (client skips on missing shader, ColorMask-0 otherwise; sidecar camera-side walls are 5.0u tall yet the frame shows low kit parapets). Owner's installed app: 0 KitRoom_ strings — unaffected.
The gate (kills the class)
Detector proven both ways: contaminated LEXAR app → `{'KitRoom_tavern'}`; owner app → `set()`.
Refs #85 / #83 (nav-truth chain). Next: box scene cleanup + rebuild through this gate → sandbox walk gate → tavern owner install.