MODEL AUDIT (#1553): Scenario model registry + provenance gate + bake-off#1554
Conversation
…-off Governance for the 7 trained Scenario models (M-ALIGN). Facts pulled from Scenario metadata, not recall. Findings: - Provenance: the adopted canonical plates (camp truegrey, tavern, crypt_armb_iter3 8.0) all use PLAIN flux.1-dev depth-CN base + Gemini 3.1 style pass — NO trained WorldOS LoRA. Only crypt-rich (6.5, honest negative) used a trained LoRA (G379). - Training quality: G379 Architectural FLUX is AI-ON-AI (8/10 training images are our own generated plates) -> DEPRECATED. v2/v4 sets are REAL PoE2/ArtStation refs. - flux.2-dev has NO controlImage/controlModality param (verified live) -> the owner's favourite v2 model is txt2img-only, cannot drive the registered path -> REFERENCE-ONLY. - Bake-off (1 crypt greybox, shared seed, blind 5-scorer panel, control 9.0 in-band, 103/120 CU): registered arms cluster 6.0-7.0 << real art 9.0; v2-Flux2 beauty ties the best styled arm but can't register; plain/exterior arms hit a text-hallucination artifact (single-seed caveat documented). Deliverables: - docs/MODEL-REGISTRY.md (role + verdict per model) + qa/model_registry.json (gate allowlist) - plate_loop.py: stamp full model chain (base+loras+scales+style) into loop JSON - promote.py: additive room provenance gate — refuse a chain using an unregistered model; NO-OP when the registry file is absent or a candidate carries no chain - tests: model-chain extraction + registry-gate (refuse/allow/additive/panel-read) Evidence: qa/evidence/model-audit/bakeoff/ (frames + bakeoff_panel_verdict.json)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1554 - MODEL AUDIT (#1553): Scenario model registry + provenance gate + bake-off evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1554 Review URL: #1554 (review) |
![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: #1554 - MODEL AUDIT (#1553): Scenario model registry + provenance gate + bake-off
Head: d7095471905a50dfd14ed00c578a6d09d89c0c26 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
qa/evidence/model-audit/bakeoff/arm_a_base_plain_flux.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_a_styled_plain_flux.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_b_base_g379_interior.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_b_styled_g379_interior.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_c_base_rswe_exterior.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_c_styled_rswe_exterior.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_d_flux2_v2_beauty.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/arm_d_flux2_v2_beauty_view.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/bakeoff_panel_verdict.json |
added | +53/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/panel/README.md |
added | +5/-0 | Documentation | Low |
qa/evidence/model-audit/bakeoff/panel/image_1.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/panel/image_3.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/panel/image_4.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/panel/image_5.png |
added | +0/-0 | Changed file | Low |
qa/evidence/model-audit/bakeoff/panel_mapping.json |
added | +1/-0 | Changed file | Low |
qa/model_registry.json |
added | +94/-0 | Changed file | Moderate: validated P3 finding |
qa/plate_loop.py |
modified | +109/-1 | Changed file | Moderate: validated P3 finding |
qa/test_plate_loop.py |
modified | +59/-0 | Test coverage | Low |
qa/test_visual_promotion_gate.py |
modified | +93/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 0, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Runtime correctness: 1
- Security boundary: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - WorldOS repo profile implies Unity runtime risk. 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: #1553, #1546.
Suggested labels: docs, tests.
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 "model_ids" not in declared: | ||
| declared = dict(declared) | ||
| declared["model_ids"] = _collect_chain_model_ids(declared) | ||
| return declared |
There was a problem hiding this comment.
P3: Declared model_chain returned by reference (minor aliasing)
In read_model_chain, when a config-declared model_chain already contains model_ids, the function returns the declared dict by reference (cfg.raw["model_chain"]) without copying. Downstream the chain is serialized into the gallery row and contract.json (phase1, lines ~824-835). Serialization is read-only today so this is not a live bug, but it aliases the live config object into the written JSON structures. If any future code mutates the returned chain (e.g. stamping a resolved asset id), it would mutate the caller's config in place. The copy-on-derive branch (line 316) already does dict(declared) — applying the same shallow copy on the early-return branch (when model_ids is already present) would make the contract uniform and avoid a subtle future aliasing footgun.
Category: Runtime correctness
Why this matters: Provenance objects that double as live config are a classic source of mutation surprises; this is a cheap preventive consistency fix, not a current defect.
| "verdict": "DEPRECATED" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
P3: New provenance gate is opt-in only until this file is trusted by callers
The model-provenance gate is correctly additive (no-op when qa/model_registry.json is absent), and that contract is well-tested (test_promote_batch_additive_when_registry_file_absent, test_model_registry_gate_noop_when_registry_absent). The trade-off to flag: because the gate only bites when the registry file exists AND a nomination declares a model_chain, any room plate minted before plate_loop stamped provenance (legacy) or any nomination hand-authored without model_chain bypasses the gate entirely. This is the documented additive design, not a defect — but it means the gate does not, by itself, prevent ad-hoc model selection for the legacy/hand-authored path until nominations uniformly carry model_chain (plate_loop now stamps it, but direct nominations.jsonl authoring does not). Consider tracking follow-up enforcement (e.g. require model_chain on room nominations once the registry is considered authoritative) in the #1553 issue rather than in this PR.
Category: Security boundary
Why this matters: A provenance gate that is trivially bypassed by omitting the field it checks gives a false sense of structural control; worth a tracked follow-up so the enforcement ratchet is explicit.
![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: d709547190
ℹ️ 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".
| if meta.get("lora"): | ||
| loras.append({"id": meta["lora"], "scale": meta.get("lora_scale")}) | ||
| _add(meta["lora"]) |
There was a problem hiding this comment.
For generate_room-controlnet metadata, the top-level lora is only the recipe default; the ControlNet request sends LoRAs from controlnet.loras instead, and current ControlNet metas commonly have that list empty. By unconditionally adding meta["lora"], plain flux depth-ControlNet plates get stamped as using model_MB22..., which corrupts the owner-facing provenance/gate and can falsely reject future valid ControlNet plates whose unused recipe LoRA is not registered.
Useful? React with 👍 / 👎.
| passed=False) only when a chain IS present and references a model absent from the registry.""" | ||
| if registry is None: | ||
| return {"ran": False, "passed": True, "reason": "no model registry file (default-allow)"} | ||
| chain = nom.get("model_chain") or panel.get("model_chain") |
There was a problem hiding this comment.
phase1 stamps model_chain into <out_dir>/panel/contract.json/gallery rows, but this fallback reads the JSON loaded from a room nomination's source_path (the completed visual-gate panel), and phase2 never copies the contract's chain into that panel. A normal room nomination following the documented class + source_path flow therefore reaches the legacy no-op path even for newly generated plates, so unregistered/ad-hoc models are not blocked unless the operator manually duplicates model_chain on the nomination.
Useful? React with 👍 / 👎.
| style_pass = meta.get("style_pass") if isinstance(meta.get("style_pass"), dict) else None | ||
| style_out: Optional[dict] = None | ||
| if style_pass: |
There was a problem hiding this comment.
When generate_room.py --layered produces the final plate, the Gemini pass2/pass3 edits live under meta["layered"], not meta["style_pass"]. This chain builder only inspects style_pass, so layered plates omit the actual edit models from model_ids; if the layered recipe is switched to an unregistered editor, the provenance gate will still pass because it never sees that model.
Useful? React with 👍 / 👎.
| explicit = chain.get("model_ids") | ||
| if isinstance(explicit, list): | ||
| return [x for x in explicit if isinstance(x, str) and x] |
There was a problem hiding this comment.
When a chain carries model_ids, the gate returns that list without checking the structured base_model/loras/style_pass fields. For hand-declared chains or stale panel JSONs, an incomplete model_ids list can omit an unregistered LoRA while still including it under loras, causing the provenance gate to pass a model the chain itself records as used.
Useful? React with 👍 / 👎.
|
Architect review: APPROVED. Gate verified additive/no-op-safe (registry-absent → default-allow; chainless legacy → no-op; refuses only declared-unregistered chains — mirrors the _paint_drift_gate contract). Registry verdicts ratified: G379 DEPRECATED (AI-on-AI confirmed, 8/10 own plates), v2-Flux2 REFERENCE-ONLY (no controlImage on flux.2-dev, schema-verified), canonical chain = flux.1-dev+depth-CN → Gemini-3.1. One follow-up for #1553 (not blocking): consider a |
1 participant
Closes #1553 (M-ALIGN model audit). Facts pulled from Scenario metadata (
model_get/asset_get/assets_list/model_schema_get), not recall.Provenance — which model made each canonical plate (owner's exact question)
flux.1-devdepth-CNflux.1-devdepth-CNasset_2Bc7…)crypt_armb_iter3(8.0)flux.1-devdepth-CNflux.1-devdepth-CNG379@0.85 (asset_rYC4…)Answer: the tavern/camp bases applied NO trained LoRA — plain flux.1-dev + depth ControlNet. The RsWE exterior-LoRA camp frames are a separate, unadopted lane. The only canonical-adjacent plate using a trained LoRA (crypt-rich) underperformed the LoRA-free incumbent.
Training-set quality
G379Architectural FLUX is AI-ON-AI — 8 of 10 training images are our OWN generated plates (camp_clearing_night_v1/v2,crypt_dense_v1,crypt_firelit_v2,church_firelit_v1,cc2_nave,tavern_firelit_v1,seedream_undercroft). Owner suspicion confirmed → DEPRECATED.J97) and v4 (ng6) are REAL refs (poe2_001..022.jpg/ curated Fandom+ArtStation) → the Crypt cutover: replace the legacy drift plate with a registered true-greybox crypt (playability-first) #1546 retrain seed corpus.FLUX.2 ControlNet check (Task 4)
model_schema_get("model_bfl-flux-2-dev")exposes nocontrolImage/controlModality/controlStrength— onlyreferenceImages. FLUX.2 does not support depth ControlNet today, so the owner's highest-rated model (J97v2, real refs, txt2img-only) cannot drive the registered geometry-locked path → REFERENCE-ONLY.Bake-off (one fixed crypt greybox, shared seed 12345, blind 5-scorer panel, 103/120 CU)
Control in-band → panel valid. Registered arms cluster 6.0–7.0, far below real art (9.0). v2-Flux2 (D) ties the best styled arm and is defect-clean — top quality, but unregisterable. Arms A/C were dragged under the adoption bar by a hallucinated text/signature artifact on the Gemini pass — a per-seed defect, not proof the interior LoRA beats plain flux (the LoRA-free incumbent scores 8.0). Single-seed caveat documented in
bakeoff_panel_verdict.json. Frames embedded below.Bake-off frames
qa/evidence/model-audit/bakeoff/arm_a_styled_plain_flux.pngarm_b_styled_g379_interior.pngarm_c_styled_rswe_exterior.pngarm_d_flux2_v2_beauty.pngDeliverables
docs/MODEL-REGISTRY.md— one row/model (base, training-data class, measured results, ROLE, verdict CANONICAL/REFERENCE-ONLY/CANDIDATE/DEPRECATED). Never delete a model (account rule) — deprecate here.qa/model_registry.json— machine-readable allowlist the gate checks.qa/plate_loop.py— stamps the full model chain (base + LoRAs + scales + style-pass model) into each loop JSON row + panel contract, fromscenario_meta.json(or a config-declaredmodel_chain).tools/library/promote.py— room provenance gate: REFUSES a candidate whosemodel_chainuses a model absent fromqa/model_registry.json. Additive — a NO-OP when the registry file is absent (default-allow) or a candidate carries no chain (legacy), mirroring the existing_paint_drift_gate.Tests
New: model-chain extraction (
test_plate_loop.py) + registry gate refuse/allow/additive/panel-read (test_visual_promotion_gate.py).81 passedon the three touched suites; the 1 remaining failure (test_committed_visual_registry_matches_builder) is pre-existing and environmental (LEXAR-mounted bg2ee frames), confirmed by re-running on clean main.Scenario spend
103 CU of the 120 cap (4 base gens + 3 Gemini passes; per-gen dry-run priced). No training, no deletion. Usage API returned 403 (role lacks
GET /usages); CU tracked from each job'scuCost.Feeds #1546: retrain on REAL refs (v4/v2) only, on a flux.1/z-image base (flux.2 can't register); never on our own generated plates (that made G379 a dead end).