feat(scenes): accept @ref target-override keys + engine version gate (labelle-engine#801) - #650
Conversation
…e (labelle-engine#801) Scene/prefab override maps may now carry "@<ref>" keys (labelle-engine#801) that patch ref-named entities nested inside a referenced prefab's body. Assembler-side support: - pack rewrite: pass 1 moves flat @ pairs into the synthesized wrapper (leaving them outside would manufacture a hybrid entry and the engine would drop the target); pass 2 opens a component map for @ values in both flat and wrapped shapes, so pack-local component names under @ get the namespace rewrite (the #801 silent-miss class). - scene_name_lint: @ keys are structure, their CONTENTS are component refs (bare pack names under @ get the did-you-mean finding). - scene_manifest: @ counts as a flat entity-shape key (allowed at a flat reference root; trips HybridForm when mixed with a wrapper). - version gate: generate hard-errors when any scene/prefab uses @ keys but the pinned engine predates 2.11.0 — compat checking is MAJOR-only, so an old engine would silently DROP @ keys, which is the exact failure mode #801 exists to kill. Unparseable pins (local:, branch) pass permissively.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds ChangesTarget Override Support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Generation
participant SceneManifest
participant SceneFiles
Generation->>SceneManifest: Check engineSupportsTargetOverrides
Generation->>SceneManifest: Find target-key usage
SceneManifest->>SceneFiles: Read scene and prefab sources
SceneFiles-->>SceneManifest: Return source text
SceneManifest-->>Generation: Return first matching path or no match
Generation-->>Generation: Abort or continue code generation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/codegen/scan/pack_refs.zig (1)
940-945: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the target pair is inside
overrides.The
target_at > wrapper_atcheck also passes if@slotis emitted after the closingoverridesbrace. That output recreates the wrapper-plus-flat-target failure this test intends to prevent.Assert the complete nested fragment, or parse the output and inspect the
overridesobject.Proposed test assertion
- const wrapper_at = std.mem.indexOf(u8, out, "\"overrides\": {").?; - const target_at = std.mem.indexOf(u8, out, "\"`@slot`\":").?; - // Exactly one wrapper, with the @ pair inside it. + // Exactly one wrapper, with the @ pair inside it. try std.testing.expectEqual(`@as`(usize, 1), std.mem.count(u8, out, "\"overrides\": {")); - try std.testing.expect(target_at > wrapper_at); + try std.testing.expect(std.mem.indexOf( + u8, + out, + "\"overrides\": { \"citizens__Worker\": { \"hp\": 1 }, \"`@slot`\": { \"Position\": { \"x\": 1 } } }", + ) != null);🤖 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 `@src/codegen/scan/pack_refs.zig` around lines 940 - 945, Strengthen the assertions in the test around the output generated by the scan flow so the "`@slot`" target is verified to be inside the "overrides" object, not merely after its opening text. Replace the target_at > wrapper_at check with an assertion for the complete nested fragment or parse out and inspect the overrides object, while preserving the existing single-wrapper and citizens__Worker checks.
🤖 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 `@src/codegen/scan/pack_refs/common.zig`:
- Around line 146-148: Update isTargetKey and the related isFlatComponentKey
classification flow to decode JSONC member-name escapes before checking key
prefixes, so escaped names such as "\u0040slot" are treated like their decoded
forms. Retain the original source byte spans for emitted output and rewrites.
In `@src/root.zig`:
- Around line 576-581: The version-gated target-key check around
scene_manifest.findTargetKeyUsage must include pack prefab files staged by
generate_phases.loadPackScans, not only game-root prefab_names. Move the gate
until after pack prefab staging completes, or collect and scan every pack prefab
source before applying the gate, while preserving the existing error behavior
for unsupported engine versions.
In `@src/scene_manifest.zig`:
- Around line 960-973: Update the quoted-key scan around isTargetKey so it
decodes JSON string escapes before checking the key, matching the JSONC parser’s
decoded representation; preserve escaped-quote handling and colon detection. Add
coverage for a key encoded as "\u0040slot" and for an escaped @ value to verify
the safety gate and override behavior.
- Around line 1033-1040: The prefab compatibility gate in findTargetKeyUsage
must not skip files that exceed the 1 MiB read limit. Handle readFileAlloc’s
error.StreamTooLong as a compatibility failure (or replace the bounded read with
a streaming scan), ensuring oversized prefabs are rejected when they may contain
target keys while preserving normal cleanup and scanning behavior.
---
Nitpick comments:
In `@src/codegen/scan/pack_refs.zig`:
- Around line 940-945: Strengthen the assertions in the test around the output
generated by the scan flow so the "`@slot`" target is verified to be inside the
"overrides" object, not merely after its opening text. Replace the target_at >
wrapper_at check with an assertion for the complete nested fragment or parse out
and inspect the overrides object, while preserving the existing single-wrapper
and citizens__Worker checks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bdac998-b12d-48d5-ba14-9fe3eaac1557
📒 Files selected for processing (8)
src/codegen/scan/pack_refs.zigsrc/codegen/scan/pack_refs/common.zigsrc/codegen/scan/pack_refs/pass1.zigsrc/codegen/scan/pack_refs/pass2.zigsrc/root.zigsrc/scene_manifest.zigsrc/scene_manifest_test.zigsrc/scene_name_lint.zig
|
@codex review |
![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: 783957f226
ℹ️ 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".
| const hit: ?[]const u8 = | ||
| try scene_manifest.findTargetKeyUsage(allocator, scenes_target, jsonc_scene_names) orelse | ||
| try scene_manifest.findTargetKeyUsage(allocator, prefabs_target, prefab_names); |
There was a problem hiding this comment.
When an engine older than 2.11.0 is pinned and a light pack ships a prefab using an @ target override, this gate scans only the game-root scene and prefab name lists. Pack prefabs are not copied and exposed through pack_scans until loadPackScans later in generate, so the gate misses them and the old engine silently drops their targets—the exact failure the gate is intended to prevent. Scan pack prefab sources as well, or run the gate after pack staging.
Useful? React with 👍 / 👎.
| const content = src[content_start..j]; | ||
| if (isTargetKey(content) and nextSignificantIsColonAt(src, j + 1)) return true; |
There was a problem hiding this comment.
For projects pinned below 2.11.0, this treats every JSON object key beginning with @ as target-override syntax, even inside opaque component payloads or metadata. For example, { "components": { "Config": { "@id": "x" } } } does not use a target override—the @id key is ordinary component data—but sourceUsesTargetKeys returns true and generation hard-fails. Track JSON scopes, or inspect the parsed entity/override structure, so payload keys do not trigger the version gate.
Useful? React with 👍 / 👎.
| allocator.free(rel); | ||
| continue; | ||
| }; | ||
| defer allocator.free(source); |
There was a problem hiding this comment.
When the pinned engine is below 2.11.0 and no early target-key hit is found, this defer keeps every successfully read JSONC buffer alive until findTargetKeyUsage returns rather than releasing it after that iteration. Consequently, generation's peak memory grows with the total size of all scenes or all prefabs being scanned (each file may be up to 1 MiB), so sufficiently large projects can exhaust memory merely running the version gate. Free source on the no-hit path inside the loop instead of deferring all frees to function exit.
Useful? React with 👍 / 👎.
…imit hardening (round 1) - sourceUsesTargetKeys moved to scene_name_lint's scope-walk model: payload @ keys (e.g. Config.@id data) no longer false-positive the gate; JSON-escaped \u0040 spellings now DO trip it - version gate scans pack SOURCE trees (prefabs/ + scenes/) via new findTargetKeyUsageInTree — pack files aren't staged when the gate runs, so a light pack's @ override slipped past before - findTargetKeyUsage: per-file buffer free (peak memory = one file), StreamTooLong is a hard error instead of a silent gate bypass, ceiling raised to 16 MiB
|
Round 1 fixes pushed (2f1d490):
Declined with reason — decode escapes in the rewrite walkers (CodeRabbit, common.zig): raw-byte key classification is a pre-existing convention shared by PascalCase detection across all five walkers ( Full suite green (the |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f1d4900fd
ℹ️ 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".
| pub fn isTargetKey(name: []const u8) bool { | ||
| return name.len > 1 and name[0] == '@'; | ||
| } |
There was a problem hiding this comment.
When a pack uses the valid JSON spelling "\u0040slot", the engine decodes it to @slot, and the new version-gate scanner explicitly recognizes that spelling. However, the pack-rewrite scanners pass the raw, unescaped key to this function, so it returns false: pass 2 treats the target's value as opaque payload and does not namespace pack-local component keys, while pass 1 can leave the target outside a synthesized overrides wrapper. Thus a supported-engine build can silently lose the target patch or its pack components; recognize/decode the escaped spelling consistently in the rewrite walkers.
Useful? React with 👍 / 👎.
| for (pack_entries.items) |entry| { | ||
| const src_dir = try entry.resolveSrcDir(allocator, game_dir); | ||
| defer allocator.free(src_dir); | ||
| for ([_][]const u8{ "prefabs", "scenes" }) |sub| { |
There was a problem hiding this comment.
If a light pack contains an auxiliary scenes/*.jsonc file with an @ key and the project pins an older engine, this loop now hard-fails generation even though pack scenes are not part of the pack runtime surface. The actual scanPack path in src/root/pack_scan.zig copies and registers components, events, prefabs, and hooks only (lines 182–198); it never stages scenes. Scan only the pack prefab tree that can reach the generated game so unused example or fixture scenes cannot impose an engine-version requirement.
Useful? React with 👍 / 👎.
…s pack prefabs only - common/lint isTargetKey now recognize the raw \u0040 spelling, so the rewrite walkers agree with the version-gate scanner: pass 1 moves an escaped target into the wrapper and pass 2 namespaces its contents (the PascalCase analog stays tracked in #651) - the pack version gate scans pack prefabs/ only — scanPack never stages a pack's scenes/, so an example scene must not impose an engine-version requirement
|
Round 2 pushed (see latest commit) — both codex P2s addressed:
Full suite green (same single pre-existing local |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b062fa812
ℹ️ 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 (c == '{') { | ||
| if (sp >= stack_buf.len) return false; // pathological nesting | ||
| stack_buf[sp] = childScope(if (sp > 0) stack_buf[sp - 1] else null, pending_key); |
There was a problem hiding this comment.
With an engine pin below 2.11.0, a prefab containing 256 nested objects or arrays in an opaque component payload causes this function to return false immediately, so a later "@slot" override is never examined. This nesting is valid JSONC, and pack prefabs are not otherwise parsed by the assembler, so the version gate permits generation and the old engine can silently drop the target override. Treat stack exhaustion as usage/an error, or skip the balanced payload without abandoning the rest of the file.
Useful? React with 👍 / 👎.
| if (c == '{') { | ||
| if (sp >= stack_buf.len) return false; // pathological nesting | ||
| stack_buf[sp] = childScope(if (sp > 0) stack_buf[sp - 1] else null, pending_key); |
There was a problem hiding this comment.
For a legacy root-wrapper pack prefab with a valid first "root" member and a later duplicate root containing "@slot", this scanner classifies both root values as entities and rejects engine pins below 2.11.0. The existing rewriteWrappedShapeRefs routing confirms that the engine uses only the first root (Object.get is first-match) and deliberately treats later duplicates as opaque dead data, so this is not runtime use of the new syntax. Mirror the rootWrapperValueOpen/.file_container routing when scanning for target keys.
Useful? React with 👍 / 👎.
| const min = std.SemanticVersion.parse(MIN_ENGINE_FOR_TARGET_OVERRIDES) catch unreachable; | ||
| const pin = std.SemanticVersion.parse(engine_version) catch return true; | ||
| return pin.order(min) != .lt; |
There was a problem hiding this comment.
When engine_version is a branch name such as 2.10.0-feature, config.isSemverVersion and versionToGitRef classify it as a branch and resolve it verbatim, but std.SemanticVersion.parse accepts the same text as a prerelease below 2.11.0. Consequently, a development branch that already contains target-override support is hard-rejected despite this function's documented policy of permitting branch pins. Check the repository's release-version predicate before performing the semantic comparison.
Useful? React with 👍 / 👎.
…ch pins classified by isSemverVersion - sourceUsesTargetKeys returns true (fail closed) past the 256-frame scope stack: an unscannable file must not bypass the version gate - engineSupportsTargetOverrides consults config.isSemverVersion first so a digit-leading branch pin (2.10.0-feature) resolves as the git ref it is, not as a prerelease below the minimum
|
Round 3 pushed — 2 of 3 codex findings fixed (fail-closed on scope-stack overflow, with a 300-deep nesting test; branch pins classified via Declined with reason — exclude dead duplicate roots: a file with two Per the review policy this was the final fix round — merging on green once CI + current reviews settle (after engine #802 merges + releases). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24dae7adce
ℹ️ 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".
| // pins are compared; everything else is permissive. | ||
| if (!config.isSemverVersion(engine_version)) return true; | ||
| const min = std.SemanticVersion.parse(MIN_ENGINE_FOR_TARGET_OVERRIDES) catch unreachable; | ||
| const pin = std.SemanticVersion.parse(engine_version) catch return true; |
There was a problem hiding this comment.
When engine_version is a numeric release ref such as 2.10, config.isSemverVersion classifies it as a release and versionToGitRef resolves it as v2.10 (src/config.zig:518-546), but SemanticVersion.parse requires the full semantic-version shape and this catch reports support. If that tag points to an engine older than 2.11.0, generation therefore skips the gate and the engine silently drops any @ overrides. Once the release predicate succeeds, normalize abbreviated versions or fail closed rather than treating parse failure like a branch pin.
Useful? React with 👍 / 👎.
| const scope = if (sp > 0) stack_buf[sp - 1] else .entity; | ||
| if (scope == .entity or scope == .component_map) { | ||
| if (isTargetKey(content)) return true; |
There was a problem hiding this comment.
When a structural key is validly JSON-escaped, such as { "prefab": "base", "over\u0072ides": { "@slot": { "Worker": {} } } }, the engine and std.json decode it to overrides, but this raw walker retains over\u0072ides as pending_key; childScope consequently marks the following object as payload and this target check never runs. With an engine pin below 2.11.0, generation then permits a real target override that the engine silently drops. The pack rewrite uses the same raw structural-key routing and also fails to namespace Worker on supported engines, so structural keys need decoding before scope classification rather than decoding only the leading @ spelling.
Useful? React with 👍 / 👎.
|
Round-4 codex triage (PR currently held open on the maintainer's call, pending the engine v2.11.0 release):
|
…-shaped unparsables The queued pre-merge fix from codex round 4: `2.10` is release-shaped (isSemverVersion) but unparsable by SemanticVersion — treating that as a permissive branch pin let an old-release pin bypass the gate. Normalize `X.Y` to `X.Y.0`; anything release-shaped that still fails to parse fails closed.
|
Hold lifted — engine v2.11.0 is released ( |
1 participant
Assembler counterpart of labelle-toolkit/labelle-engine#802 (RFC on labelle-toolkit/labelle-engine#801):
"@<ref>"override keys that patch entities nested inside a referenced prefab's body.Changes
codegen/scan/pack_refs):@pairs into the synthesized wrapper — leaving one outside would manufacture a hybrid entry the author never wrote, and the engine would then warn wrapper-wins and drop the target;@values in both flat and wrapped shapes, so a pack-local component name under@gets the namespace rewrite (this was the load-bearing piece: without it,industry__TendableWorkstationunder@tendersilently misses the rewrite — the same silent-miss class as the namespaced-string-refs trap).scene_name_lint:@keys are structure, their contents are component refs — a bare pack name inside a@patch gets the did-you-mean finding.scene_manifest:@counts as a flat entity-shape key — allowed at a flat reference root, tripsHybridFormwhen mixed with anoverrides:wrapper.@keys but the pinned engine predates 2.11.0. Compat checking is MAJOR-only (labelle-cli#269), so "new assembler + old engine" is a legal pin combo — and an old engine silently drops@keys, which is the exact failure labelle-engine#801 exists to kill. Unparseable pins (local:dev overrides, branch pins) pass permissively. Detection is a colon-lookahead textual scan, so@refvalues ("target": "@storage") never trip it.Tests
10 new tests across
pack_refs,scene_name_lint,scene_manifest_test: rewrite-under-@(flat + wrapped), pass-1 wrapper migration, lint ref collection, HybridForm with@, top-level acceptance, usage detection (keys vs values vs comments), version predicate incl. permissive pins.Note:
flow_catalog.zig:587(emitFlowCatalogSidecarround-trip) fails in my local environment on cleanorigin/maintoo — pre-existing local-env issue, unrelated; CI should be authoritative.Sequencing
Engine PR labelle-toolkit/labelle-engine#802 first (releases as 2.11.0), then this;
MIN_ENGINE_FOR_TARGET_OVERRIDESis pinned to2.11.0— bump if the engine release slips to a later minor.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
@target overrides in flat-form scenes, prefabs, and nested components.Bug Fixes
Tests