Repository navigation
feat(packs): rewrite FLAT-shape pack prefabs into the wrapped namespaced form (#513) - #515
Conversation
…ced form (#513) Closes the #440 gap: rewritePackLocalRefs namespaced a pack's own component keys only in the wrapped "components"/"overrides" shape, while engine RFC #596 makes the FLAT shape (PascalCase keys at entity scope) the recommended one. Worse, an in-place flat rewrite can never work: the engine classifies flat keys by case, so a namespaced <pack>__Pascal key demotes to a structural key and is dropped with zero log signal (observed on the FP pilot, flying-platform-labelle#573 -- sky entities never attached, script views empty). Fix is option (b) from the ticket thread -- self-contained in the assembler, no engine release needed: a normalization pre-pass (wrapFlatEntityComponents) converts any flat entity that declares pack-local components into the wrapped shape during the pack copy; the existing scope-tracked walk (now pass 2, rewriteWrappedShapeRefs) then namespaces keys exactly as before, so the rewrite logic is not duplicated. Shape rules (each mirroring the engine's unified_format.zig): - ALL PascalCase keys move together into ONE synthesized wrapper at the first moved key's position (the engine's "wrapper wins" rule would DROP any key left flat beside a wrapper); lowercase structural keys (prefab/children/meta/ref) stay at entity scope in order. - Wrapper spelling: "overrides" for prefab references (string-valued "prefab" -- the warning-free patch spelling; "components" on a ref is a warned legacy synonym per RFC #560), "components" for inline. - An entity with no pack-local flat key stays byte-identical: flat engine-only entities load fine as-is, so the copy stays minimal-diff. - A mixed wrapper+flat entity is left verbatim so the engine's "wrapper wins" warn-once still fires on the copy (behavior parity with the same file at game root). - Payload decoys ({ "Spawner": { "SkyBody": 3 } }) are copied byte-verbatim; JSONC line/block comments ride along with their pair. - Safety valve: the pass only rewrites files it can fully account for (strict balanced-container scanning, nesting cap, object root); anything else -- including RFC #596 file-as-array bundles -- falls back to the untouched input and pass 2 behaves exactly as today. Pack-local "prefab" VALUE rewriting keeps working in both shapes. Tests: 10 new unit cases on rewritePackLocalRefs (flat wrap + namespacing, payload decoy, byte-identical no-ops incl. a trailing comma, mixed-shape file with a comment between children and the LAST no-trailing-comma element FP's hand-conversion missed, the overrides-vs-components decision, nested flat root+child, comment preservation, idempotency) + 1 end-to-end scanPack fixture (flat inline prefab + flat reference child, asserted on the copied files).
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a flat-shape normalization pre-pass to pack-local JSONC rewriting so flat entity keys and prefab references are rewritten correctly, with updated docs and tests. ChangesFlat-shape prefab component key rewriting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a normalization pre-pass (wrapFlatEntityComponents) to convert flat-shape entities (RFC #596) that declare pack-local components into the wrapped shape before namespacing their keys. This prevents namespaced keys (which start with a lowercase letter) from being silently dropped by the engine's case-based flat-key classification. The changes include the recursive-descent FlatWrap JSONC parser/emitter, updated documentation, and extensive unit and integration tests to verify correct wrapping, decoy handling, comment preservation, and idempotency. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/codegen/scan.zig (1)
617-633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a regression test for "inline entity with a dead
overrideskey + flat keys" wrap path.Per the doc, an
"overrides"key on an inline (non-reference) entity doesn't block the wrap (only"components"does), so such an entity should still wrap its flat keys into"components"alongside the untouched dead"overrides". This exact branch (has_overrides=true, is_reference=false→wrapper_present=false) isn't exercised by any test in this file.Suggested test addition
test "rewritePackLocalRefs: inline entity with a dead overrides key still wraps flat keys as components (`#513`)" { const allocator = std.testing.allocator; const src = \\{ "overrides": { "unused": 1 }, "SkyBody": { "role": "sun" } } ; const out = try rewritePackLocalRefs(allocator, src, &.{"SkyBody"}, &.{}, "sky"); defer allocator.free(out); try std.testing.expect(std.mem.indexOf(u8, out, "\"components\": { \"sky__SkyBody\"") != 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.zig` around lines 617 - 633, Add a regression test for `rewritePackLocalRefs` covering an inline entity that has a dead `overrides` key plus flat component keys: this case should still set `has_overrides=true`, `is_reference=false`, and keep `wrapper_present=false` so the flat keys get wrapped into `components` while the unused `overrides` stays untouched. Extend the existing test coverage in `scan.zig` by adding a case similar to `rewritePackLocalRefs` that asserts the output contains a `components` wrapper for the flat key (e.g. `SkyBody`) even when an `overrides` object is present.
🤖 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.
Nitpick comments:
In `@src/codegen/scan.zig`:
- Around line 617-633: Add a regression test for `rewritePackLocalRefs` covering
an inline entity that has a dead `overrides` key plus flat component keys: this
case should still set `has_overrides=true`, `is_reference=false`, and keep
`wrapper_present=false` so the flat keys get wrapped into `components` while the
unused `overrides` stays untouched. Extend the existing test coverage in
`scan.zig` by adding a case similar to `rewritePackLocalRefs` that asserts the
output contains a `components` wrapper for the flat key (e.g. `SkyBody`) even
when an `overrides` object is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b9c25aa-1007-4c2c-b21c-f740b88c8248
📒 Files selected for processing (3)
src/codegen/scan.zigsrc/root.zigtest/pack_scan_tests.zig
Addresses the one CodeRabbit nitpick on #515: the `has_overrides=true, is_reference=false` -> `wrapper_present=false` branch had no regression test. An `"overrides"` key on an INLINE entity is a dead structural key the engine ignores (`entityPatch` consults `overrides` on references only), so it must not block the wrap: the flat pack key still moves into a synthesized `"components"` map -- namespaced by pass 2 -- while the dead `"overrides"` map stays at entity scope byte-verbatim (its contents are not component declarations to pass 1, and `unused` is no pack key to pass 2). Behavior verified as already matching the documented rule; test-only change, green on first run.
![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: 602e90f5ec
ℹ️ 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 (std.mem.eql(u8, p.key, "overrides")) has_overrides = true; | ||
| if (containsKey(self.component_keys, p.key)) has_local_flat = true; | ||
| } | ||
| const wrapper_present = has_components or (is_reference and has_overrides); |
There was a problem hiding this comment.
This only shows up for an inline pack prefab that has an overrides object and a flat pack-local component key, e.g. { "overrides": {...}, "SkyBody": {...} }. The repo’s RFC #596 validator treats overrides plus flat PascalCase keys as a hybrid at every entity site (src/scene_manifest.zig lines 454-472 and 534-545), but this condition ignores has_overrides unless prefab is a string, so pass 1 rewrites that hybrid into a new components wrapper instead of preserving the engine/validator diagnostic. Include has_overrides in wrapper_present so wrapper+flat inputs remain byte-stable as documented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 426ff76 — you're right, and I verified the evidence: src/scene_manifest.zig rejects overrides + flat PascalCase as error.HybridForm at every entity site with deliberately no reference check (root-block and child-entry gates), and its comment notes engine #597 applies the same gate. My wrapper_present was mirroring the engine's entityPatch accessor semantics (overrides consulted on references only), but the authoritative rule for what pass 1 may touch is the hybrid-form gate, not the accessor fallback.
Change: ANY pre-existing wrapper key (components OR overrides, inline or reference) now blocks the wrap and the entity stays byte-verbatim, so the author's text keeps tripping the HybridForm diagnostic; is_reference only picks the synthesized wrapper's spelling for pure-flat entities. The round-1 regression test is flipped to assert byte-identity, and the docs now define the rule crisply: hybrid = wrapper key and flat Pascal keys coexisting; pure flat (no wrapper key anywhere) still converts.
…ntities stay byte-verbatim (#513) Addresses the codex P2 on #515, which reverses the CodeRabbit round-1 expectation: `wrapper_present` only counted `overrides` on references (mirroring the engine's `entityPatch` accessor), so an INLINE entity mixing an `overrides` object with flat pack-local keys got its flat keys wrapped into a NEW `components` map beside the existing `overrides`. But that mix is the RFC #596 HYBRID form and this repo's own scene validator hard-rejects it at EVERY entity site with deliberately no reference check (`scene_manifest.zig`, `error.HybridForm`; engine #597 applies the same gate) -- pass 1 was masking the author's error instead of preserving the diagnostic, in violation of this PR's own stated principle. Now ANY pre-existing wrapper key (`components` OR `overrides`, reference or inline) blocks the wrap and the entity comes back byte-verbatim; `is_reference` only picks the synthesized wrapper's SPELLING for pure-flat entities. Crisp rule, now documented on `wrapFlatEntityComponents` and in the tests: HYBRID = a wrapper key AND flat Pascal keys COEXIST in the author's text (left alone); PURE FLAT = Pascal keys with no wrapper key anywhere (converted -- the flat-reference/overrides-spelling behavior is unchanged). Flips the round-1 regression test accordingly (inline overrides+flat now asserts byte-identity) and aligns the components+flat hybrid test's comment with the validator evidence.
|
@coderabbitai Heads-up on your round-1 nitpick (the "inline entity with a dead As of 426ff76 that branch no longer exists: any pre-existing wrapper key ( |
|
@codex review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "HybridForm" -C5 src/scene_manifest.zigLength of output: 5915
🐰 ᕕ( ᐛ )ᕗ ✏️ Learnings added
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review (Re-triggering — the earlier request may not have registered. Please review the current head |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…es in the pack-prefab rewrite (#516) (#521) * feat(packs): walk file-as-array bundles and legacy "root"-wrapper files in the pack-prefab rewrite (#516) Both rewrite passes — the #513 flat->wrapped normalization (pass 1) and the #440 wrapped-shape namespacing walk (pass 2) — only recognized the plain single-entity file shape, so a pack prefab authored in either of the two other engine-accepted FILE shapes silently got NO namespacing (bare component keys AND bare "prefab" refs — the #513 silent-unattach failure class via a different door): - RFC #596 file-as-array bundles: a top-level Array of sibling entities, with an optional only-`meta` header element at index 0. - Legacy "root"-wrapper files: { "root": { ...entity... } } (v1.0-v1.x, still dual-accepted by the engine). Teach the two entry points to classify the container shape before descending, mirroring the engine's unified_format.zig rules exactly (classifyTopLevel / isFileHeader / rootObject, checked at v1.66.0): - Shared byte-scanning probes `isOnlyMetaHeaderObject` / `bundleHeaderOpen` / `isRootWrapperFile` so the two passes can never disagree about what a header or a wrapper is. Header = object with `meta` and NO entity-shape key (prefab/children/components/overrides/ ref/PascalCase); wrapper = FIRST "root" entry has an object value (engine Object.get is first-match). - Pass 1: `emitRoot` dispatches to a new `emitBundle` (header element copied byte-verbatim, sibling elements walked as entities) and `emitFileContainer` ("root" value descends as the entity; children/entities arrays stay live per the engine's fileChildren partial-migration fallback; other members verbatim — the engine never reads them). - Pass 2: document-root arrays now open `.array_entities`, the header element (offset pre-computed) walks as opaque `.payload`, and a new `.file_container` scope confines the root-wrapper unwrap to the document level — a "root" key on a NESTED entity stays payload. The per-entity logic of both passes is unchanged; untouched files stay byte-identical (no-op inputs round-trip exactly, comments verbatim). Found while implementing #513 (PR #515); FP pilot context is flying-platform-labelle#573. Closes #516 * feat(packs): warn on bare pack-local component keys surviving the copy rewrite (#516 net) The generate-time net the #516 ticket calls for: after rewritePackLocalRefs runs over a copied pack prefab, no component DECLARATION position should still use one of the pack's own bare Pascal names — every such survivor silently fails to attach at load (the #513/#516 failure class). Scan the rewritten copy with the scene_name_lint reference walk (which already covers the flat, wrapped, bundle, and root-wrapper shapes) and std.log.warn each leftover with file:line:col. Survivors are real, not just hypothetical rewriter gaps: the RFC #596 HYBRID form (wrapper + flat keys mixed) is deliberately left byte-verbatim so the author's own diagnostics keep firing, and a flat key beside a "root" wrapper is dead data the engine never reads — the warning names all three possibilities. Pure helper (`scene_name_lint.findBareLocalRefs`) + warn-only driver (`warnLeftoverBareKeys` in root.zig): a diagnostic, never a gate. Ref #516 * fix(packs): honor only the FIRST "root" member — duplicates are dead data (#521 CodeRabbit) The engine's Object.get is first-match, so only the FIRST "root" entry is the root binding; a duplicated later "root" member is dead data the engine never reads. Both passes previously descended into EVERY object-valued "root" member of a wrapper file, so the duplicate would get wrapped/namespaced — breaking the "other container members stay verbatim" contract. Route the entity descent by BYTE OFFSET instead of by key: the probe becomes rootWrapperValueOpen (?usize — offset of the first "root" entry's object-value `{`, null when not a wrapper), mirroring the existing header_open pattern so both passes key off the same probe: - Pass 2: `i == root_open` opens `.entity`; childScope's `.file_container` arm is now unconditionally `.payload`, so the duplicate lands there and stays dead-data verbatim. - Pass 1: emitFileContainer takes root_open and descends only into the pair whose value_start equals it; other "root" pairs copy verbatim. Regression test: duplicated "root" key — first wrapped + namespaced, second byte-verbatim (bare key included), exactly one namespacing in the file, and idempotency across a second run. Ref #516 * fix(packs): errdefer the findBareLocalRefs result list (#521 Gemini) A no-op under the function's documented arena contract (all current callers pass an ArenaAllocator, mirroring collectComponentRefs), but the errdefer keeps the growth path leak-free should a future caller pass a general-purpose allocator. Ref #516 * feat(packs): warn when a bundle header carries a legacy "entities" list (#521 codex P2) Codex asked the header probe to DISQUALIFY `entities` — that would diverge from the engine: `isFileHeader` (labelle-engine src/jsonc/unified_format.zig @ v1.66.0) disqualifies exactly prefab/children/components/overrides/ref/PascalCase and explicitly tolerates other lowercase keys, so `{ "meta": ..., "entities": [...] }` IS a header to the engine too, and `classifyTopLevel` extracts only its `meta` — the entities list is dead data the loader never reads. Rewriting inside it would mutate bytes the engine consumes as metadata, so the probe keeps exact parity and the element stays byte-verbatim. The underlying concern is real though — that shape is almost certainly an authoring mistake — so convert it into the generate-time signal it deserves: new engine-parity probe `bundleHeaderLegacyEntitiesOffset` plus a warning in the #516 net driver (file:line:col, "dead data that never loads; move them into top-level bundle elements"). Test: probe hit on a header with entities, null for entity-first bundles and clean headers, and the rewrite still skips the whole header verbatim while the live sibling namespaces. Ref #516
1 participant
Closes #513
What
rewritePackLocalRefs(#440) namespaced a pack prefab's own component keys only in the wrapped"components"/"overrides"shape. Engine RFC #596 makes the flat shape (PascalCase component keys at entity scope) the recommended one, and real pack prefabs use it — so a flat pack prefab's components silently never attached after the copy. Field evidence: Flying-Platform/flying-platform-labelle#573 (packs/sky pilot on assembler 0.72.0 — sky entities never attached, script views empty, zero log signal; FP converted its sky prefabs to the wrapped shape as an interim).Design: option (b) — flat → wrapped during the pack copy
Per the ticket thread, option (a) (rewrite flat keys in place) is a dead end without an engine release: the engine's flat loader classifies entity-scope keys by case (
unified_format.zigisPascalCase), so a namespacedsky__SkyBodykey starts lowercase, demotes to a structural key, and is dropped silently — the RFC #596 unknown-component warn-once doesn't fire either (Pascal-cased unknowns only). Option (b) is self-contained in the assembler: the wrapped shape has no case rule, so it's the only shape a<pack>__key can attach in today.Implementation is a two-pass composition inside
rewritePackLocalRefs, so the namespacing logic is not duplicated:wrapFlatEntityComponents: a recursive-descent normalizer over the copy. For each entity (file root orchildren/entitieselement) that declares at least one pack-local component flat and has no explicit wrapper, ALL PascalCase keys (engine components likePositionAND pack-local ones) move — values byte-verbatim — into one synthesized wrapper at the first moved key's position. Moving all of them together is load-bearing: the engine's "wrapper wins" rule drops any key left flat beside a wrapper. Structural lowercase keys stay at entity scope."prefab"values, in both shapes.Behavior/parity decisions (each mirrors
entityPatchin the engine'sunified_format.zig):"overrides", not"components":entityPatchaccepts"components"on a reference but warns it as a legacy synonym (RFC tilemap: scan scenes for Tilemap + embed .tmx and tileset images (T2 Phase 4) #560);"overrides"is the warning-free patch-map spelling. Inline entities wrap as"components". Reference detection = string-valued"prefab"key, exactly like the engine'sis_reference.Test coverage
Unit (
src/codegen/scan.zig, all through the publicrewritePackLocalRefs):{ "Spawner": { "SkyBody": 3 } }) untouched — both inside a wrapping entity and as a no-op file"overrides", never"components"End-to-end (
test/pack_scan_tests.zig):scanPackon a fixture pack with a flat inline prefab and a flat reference child; asserts the on-disk copies.zig build test: 46/46 steps, 1204 pass (4 pre-existing skips).Follow-ups observed (out of scope, pre-existing)
"root"-wrapper file shape are walked by NEITHER pass (pass 2's scope walk predates them too) — a pack prefab written in those shapes gets no namespacing at all today. Worth a ticket if packs ever author them.<pack>__Pascalkeys (prerequisite for v2.0, which drops the wrapper), pass 1 can flip to an in-place flat key rewrite; the doc comments call this out.Summary by CodeRabbit
components/overridesform during copy to ensure correct namespacing and reference updates.