fix(scenes): one engine-parity key classifier for every scene walker (#651, #652) - #798
Conversation
…651, #652) Every textual scene walker (pack rewrite pass 1/2, scene_name_lint, the @ version gate) and the scene manifest now classify keys through one module, src/scene_keys.zig, byte-parity with labelle-engine's unified_format.zig and its JSONC parser's escape set. #652: flat-form pack-namespaced `<prefix>__<Pascal>` keys (engine#806) are component keys. They are accepted at a scene's top level, count toward flat-form / hybrid detection, are collected by the name lint, and ride into pass 1's synthesized wrapper instead of being left flat beside it. #651: the engine's JSONC parser does not decode \uXXXX; it fails the whole file with InvalidEscape. Every escape it does accept decodes to a byte that can't change a key's classification, so raw spans classify exactly like the engine's decoded keys. Keys with a rejected escape are inert in the walkers, the old raw-byte special case for the escaped-@ spelling (which assumed the engine decodes it) is gone, and parseSceneSource rejects such files at build time, before std.json (which does decode \u) gives them a meaning the engine never will. The @ version gate moves to scene_target_gate.zig (re-exported) to keep scene_manifest.zig under 1000 lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 101 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThis change adds shared scene-key classification and escape validation. Manifest checks, name linting, and pack-reference rewriting use the shared rules. Target-key detection, engine-version checks, and JSONC file scans are provided through ChangesScene key handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to No actionable issue is established that would block merging. Engine-parser escape parity has not been independently confirmed. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
A rabbit checks each key at dawn Comment |
|
@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: da31429af9
ℹ️ 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".
| // loader warn-onces unknown component names at runtime; the scan | ||
| // can't see the component registry here, so any component-SHAPED key | ||
| // is accepted (the audit's option C resolution). | ||
| if (isFlatComponentKey(key)) return true; |
There was a problem hiding this comment.
When a project pins an engine revision predating engine#806, this now accepts a game-root key such as rooms__Room, but that engine still classifies the lowercase-leading key as structural and silently drops the component. Only copied pack prefabs go through the compatibility wrapper in pack_refs; game-root scenes remain flat, and the repository has no corresponding #806 engine-version gate. Preserve the previous rejection for old pins, add a feature gate like the @ override gate, or normalize these scenes before staging.
Useful? React with 👍 / 👎.
| if (scene_keys.firstInvalidEscape(source)) |off| { | ||
| const loc = @import("scene_name_lint.zig").locOf(source, off); | ||
| stderrPrint( | ||
| "labelle-assembler: scene '{s}' line {d} col {d}: unsupported string escape. The engine's JSONC parser accepts only \\n \\t \\r \\b \\f \\\\ \\\" \\/ (no \\uXXXX) and would fail to load this file.\n", | ||
| .{ display_path, loc.line, loc.col }, | ||
| ); | ||
| return error.InvalidSceneJson; |
There was a problem hiding this comment.
This validation only runs through parseSceneSource, while game-root and staged pack prefabs are read through scanTilemapAssets, which still feeds them directly to std.json. Consequently, a prefab key such as "\u0054ilemap" is decoded as Tilemap and can fabricate a tilemap registration or fail generation with TilemapAssetNotFound, even though the engine rejects the entire prefab as InvalidEscape. Run the shared escape check in the raw prefab parsing path as well, rather than allowing different meanings for the same source depending on which scanner consumes it.
Useful? React with 👍 / 👎.
…e files in std.json scanners Codex on #798: - P1: engines before v2.11.0 (engine#806) silently drop flat `<prefix>__<Pascal>` keys at entity scope. The existing `@` version gate (same v2.11.0 minimum) now also catches them via sourceNeedsV211Keys. Wrapped namespaced keys, which old engines always loaded, don't trip it. - P2: scanTilemapAssets / scanScenePrefabRefs feed std.json, which decodes escapes the engine rejects; a file the engine can't load now yields no tilemaps / prefab refs instead of made-up ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f60047ebd
ℹ️ 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; | ||
| }, | ||
| }; | ||
| const used = sourceNeedsV211Keys(source); |
There was a problem hiding this comment.
When an engine older than 2.11 is pinned, this scans each pack's original source and rejects any flat namespaced key before rewritePackPrefabRefs runs. A pack prefab such as { "Worker": {}, "rooms__Room": {} }, where Worker is pack-local, is subsequently normalized by wrapFlatEntityComponents into a wrapped component map that old engines can load, so the new gate incorrectly aborts an otherwise compatible build. Apply the flat-namespaced check to the staged/rewritten prefab, or otherwise exclude source prefabs whose flat content will be wrapped; source scanning is still necessary for @ keys.
Useful? React with 👍 / 👎.
…ewritten copies Codex on #798: the pre-staging pack SOURCE scan rejected flat namespaced keys that pack rewrite pass 1 later wraps (which loads on every engine). The source scan is back to `@`-only; flat namespaced keys in pack prefabs are checked on <target>/packs/<pack>/prefabs after loadPackScans. Game-root scenes/prefabs (never rewritten) keep the combined check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0255756db
ℹ️ 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".
| .flat_namespaced => if (scope == .entity) { | ||
| if (isComponentKeyShape(content) and !scene_keys.isPascalCase(content)) return true; |
There was a problem hiding this comment.
When an engine older than 2.11 is pinned, a prefab such as { "root": { ... }, "rooms__Room": {} } is rejected even though the namespaced sibling has no meaning to any engine: the legacy loader selects the first root object and treats other object-valued siblings as opaque dead data. The pack rewrite models this explicitly with its file_container scope, but this walker classifies the document object as .entity, so both game prefabs and staged pack prefabs can spuriously fail the new version gate. Detect root-wrapper containers (including first-match handling for duplicate root keys) before applying the flat-namespaced check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, but it only produces a loud false-positive generate error on pre-2.11 pins for legacy root-wrapped files that are malformed anyway; nothing is silently dropped. Fix rounds for this PR are used up, so it's tracked in #805.
1 participant
Fixes #651. Fixes #652.
What changed
All scene-key classification now goes through
src/scene_keys.zig, byte-parity with labelle-enginesrc/jsonc/unified_format.zig(isPascalCase/isComponentKeyShape/isTargetKey/isFlatComponentKey) and with the engine JSONC parser's escape set (jsonc/src/parser.zigparseString). The duplicated classifiers inpack_refs/common.zig,scene_name_lint.zigandscene_manifest.zigare gone. Those files now delegate to it.#652: flat pack-namespaced keys (engine#806)
<prefix>__<Pascal>at entity scope is a component key, exactly where the engine accepts it (non-empty prefix, PascalCase after the last__):scene_manifest.isAllowedTopLevelKeyacceptsrooms__Roomat a file's top level.capacity__oops,__Roomandrooms_Roomare stillUnknownSceneKey.hasFlatEntityShapeKey/checkHybridFormcount namespaced keys: namespaced + wrapper isHybridForm, and a namespaced-only file is flat form.scene_name_lint.collectComponentRefscollects flat namespaced refs at entity scope.pack_refspass 1 moves flat namespaced pairs into the synthesized wrapper. Before this, they stayed flat beside it, and the engine then warns "wrapper wins" and drops them.#651: escaped key spellings
When I checked the premise against the engine, it didn't hold: the engine's JSONC parser does not decode
\uXXXX. It fails the whole file witherror.InvalidEscape. I checked every revision ofjsonc/src/parser.zigand ran a probe against current main. The escapes it does accept (\n \t \r \b \f \ \" \/) all decode to bytes that aren'tA-Z,@or_and don't appear in any structural key name. So a raw key span classifies exactly like the engine's decoded key. The module doc spells out why, and an exhaustive test checks it: every accepted escape at every position of a set of base keys, compared against a referencedecode.The escape rules are now shared, so:
.unloadableand is inert in every walker. The old raw-byte special case for the escaped@spelling (added in feat(scenes): accept@reftarget-override keys + engine version gate (labelle-engine#801) #650 on the assumption that the engine decodes it) is removed frompack_refs/commonandscene_name_lint. The feat(scenes): accept@reftarget-override keys + engine version gate (labelle-engine#801) #650 tests that asserted it are replaced by tests of the engine's behaviour.parseSceneSourcerejects a scene with an engine-rejected escape (keys or values, comment-aware) beforestd.jsonsees it.std.jsondoes decode\u, so without this check the assembler gave such keys a meaning (e.g. aWorkercomponent) for a file the engine can't load.sourceUsesTargetKeys/findTargetKeyUsage*move tosrc/scene_target_gate.zig(re-exported fromscene_manifest), which keepsscene_manifest.zigunder 1000 lines (it was at 1092).Tests
src/scene_keys_test.zig(wired inroot.zig) is table-driven:\uspellings are.unloadable, anddecodereturnsInvalidEscapefor them, as the engine does;@gate, and the pack-rewrite "which flat keys move into the wrapper" table (the output shape shows which path ran).Locally (Windows, Zig 0.16.0):
zig buildexit 0, andzig fmt --checkon the touched files exit 0.zig build testexits 1 only on 3 generate-based tests that fail withREPARSE_POINT_NOT_RESOLVED(Windows junctions in.zig-cache/tmp); they fail the same way on origin/main here. All the new and changed tests pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01Caqyi5Snr6yKssztYFztzZ
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
\u0040spellings as valid target keys.