fix(migrate,audit,upgrade): safe unified-format migration + honest version report (#336, #337, #338) - #339
Conversation
…rs; survive collapsed closers (#337, #338) Two defects with one shared blast radius, found while migrating flying-platform-labelle to engine 2.x. #338 — the inline `components:` wrapper is NOT legacy. The engine's case-convention rule (RFC #596) only reads PascalCase flat keys as components; pack-namespaced lowercase keys (`rooms__Room`, `industry__Storage`) are ONLY recognized inside the wrapper. Lifting them flat made the engine silently drop the components at load — no error, rooms just lose their behavior. Engine v2.0 removed `components` on prefab REFERENCES only. - transform 6 (inline components lift) removed; the wrapper is left untouched and the audit no longer reports it as legacy debt. - the overrides-on-ref lift (transform 5) now fires only when every inner key is PascalCase; wrappers holding lowercase keys are the required shape and are skipped by both migrate and audit (the audit↔migrate 1:1 count mapping is preserved). - `isEntityShapeKey` now treats `components`/`overrides` as entity content, so the directives-to-meta pass can no longer relocate a root entity's components into `meta:` — the wrapper only survives to that pass since this change, which is exactly how the crash below was triggered. #337 — `migrate unified` aborted whole projects with `UnexpectedEndOfInput` and no file name. Root cause chain: the lift exposed a lowercase key → the directive mover moved it and its line-based splice ate the file's closing brace when the entry's value ended against collapsed closers (`}}`) → the next pass re-parsed the corrupted buffer and the error propagated project-wide. - `deleteTopLevelKey`/`spliceDropEntry` extend a cut to end-of-line only when the line tail is blank or a `//` comment, so collapsed closers survive. - `transformBytes` failures are contained per file: the path and error are reported, the file is left unmodified, and the run continues. On flying-platform-labelle (30 collapsed-closer prefabs, 101 inline wrappers, ~1930 lowercase-keyed overrides wrappers): previously a hard abort; now clean — audit findings drop 2741 → 703, and every remaining migrate edit is semantics-preserving.
…ans, refresh the compatible set (#336) `upgrade --check` presented its build-time compatible set as "latest" ("update available ... run labelle upgrade all") while the set lagged five engine versions behind the newest tag — following its advice landed a project on engine 2.5.0, which cannot compile any project with a large pack (missing @setEvalBranchQuota; fixed upstream in 2.10.0 / engine#795). And `backend_package` (bgfx) was absent from the report entirely, hiding the coordinated gfx+bgfx bump the 1.26→1.27+ backend-contract boundary requires. - versions.zon: 1.26.0/2.5.0/1.28.1 → core 1.27.0 / engine 2.10.0 / gfx 1.28.5 — verified together on flying-platform-labelle (clean build from pristine caches, 227-file test suite green, three scenes run without panics or format warnings). - ProjectConfig now parses `backend_package`, and --check reports its pin with an explicit "latest not tracked by this CLI" note instead of omitting it. - Human output stops claiming "latest": pins are "behind this CLI's compatible set", with a note that the set is frozen at CLI build time and newer tags may exist. JSON shape unchanged (studio#7).
|
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:
📝 WalkthroughWalkthroughThe change makes inline ChangesUnified migration behavior
Backend package status reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant migrateFile
participant transformBytes
participant JSONCSplicing
participant Summary
migrateFile->>transformBytes: transform file bytes
transformBytes->>JSONCSplicing: preserve collapsed closers during edits
JSONCSplicing-->>transformBytes: transformed bytes
transformBytes-->>migrateFile: result or error
migrateFile->>Summary: count edits or parse_errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 95b10ea3d2
ℹ️ 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".
| .gfx = "1.28.1", | ||
| .core = "1.27.0", | ||
| .engine = "2.10.0", | ||
| .gfx = "1.28.5", |
There was a problem hiding this comment.
For bgfx projects still using the backend pin paired with gfx 1.28.1, labelle upgrade all now writes gfx 1.28.5 while leaving backend_package.version untouched: the is_all branch in src/cli/upgrade.zig only replaces the core, engine, gfx, labelle, and assembler fields. Because this change explicitly identifies the gfx bump as crossing a backend-contract boundary that requires a coordinated bgfx bump, the command can produce an incompatible project that no longer builds; include the tested backend version in the compatible set and update it together with gfx (or do not advance the gfx target).
Useful? React with 👍 / 👎.
| // would let `moveOneDirectiveToMeta` relocate the root entity's | ||
| // components into `meta:` — destroying them just as thoroughly as | ||
| // the lift did. | ||
| if (std.mem.eql(u8, key, "components")) return true; |
There was a problem hiding this comment.
When a single-root file has top-level children plus a canonical components wrapper, this new guard correctly treats the wrapper as entity content, so migration leaves the object intact. However, audit.zig's legacy_file_object_no_root detector still recognizes only PascalCase top-level keys and therefore reports that same file as a bundle that should collapse to an array. This leaves users with a false, unfixable audit finding and breaks the stated audit-to-migrate count mapping; the audit predicate must treat components as a root-entity marker too.
Useful? React with 👍 / 👎.
| } | ||
| if (updates == 0) { | ||
| try w.writeAll("all pins up to date\n"); | ||
| try w.writeAll("all pins match this CLI's compatible set\n"); |
There was a problem hiding this comment.
When a project has an unchecked package such as the newly reported backend, or a framework pin newer than the bundled target, updates remains zero even though the pin was never compared or does not equal the target. The new summary consequently prints all pins match this CLI's compatible set, contradicting the per-package backend warning and misreporting newer pins; track equality/unchecked statuses separately instead of deriving a match from the absence of known upgrades.
Useful? React with 👍 / 👎.
| // (3f) REMOVED (cli#338). The `components:` wrapper on an INLINE | ||
| // entity is a canonical engine 2.x shape, not legacy: the engine's | ||
| // case-convention rule only treats PascalCase flat keys as components, | ||
| // so pack-namespaced (lowercase) keys like `rooms__Room` exist ONLY | ||
| // inside the wrapper. Engine v2.0 removed `components` on prefab | ||
| // REFERENCES only (`legacy_components_on_ref`, 3b). |
There was a problem hiding this comment.
When users run labelle audit unification --help, the usage text still lists an inline components wrapper among the legacy patterns the command reports, even though this change removes that finding and establishes the wrapper as canonical engine 2.x syntax. The module-level pattern list makes the same obsolete migration recommendation, so users can be prompted to perform the exact unsafe lift this fix is intended to prevent; update both descriptions when removing the rule.
Useful? React with 👍 / 👎.
…ump, honest summary, audit parity
Codex P1: `upgrade all` advanced gfx across the backend-contract
boundary while leaving `backend_package.version` untouched — producing
exactly the broken gfx/bgfx pairing this PR warns about. bgfx joins the
compatible set (versions.zon → build options), `upgrade all` bumps it
together with gfx through the same downgrade guard, and `--check` now
compares the bgfx pin instead of reporting it untracked. The rewrite is
anchored at `.backend_package` so plugin pins sharing a `.version`
field name are never touched (regression test included).
Codex P2 (audit parity): `legacy_file_object_no_root` still keyed off
PascalCase-only top-level keys, so a `{children, components}` root-
entity file — which the migrator now (correctly) refuses to collapse —
got a false, unfixable finding. The predicate mirrors the migrator's
`isEntityShapeKey` (`components`/`overrides`/`prefab` mark a root
entity), restoring the audit↔migrate 1:1 mapping.
Codex P2 (summary honesty): "all pins match" was derived from the mere
absence of known upgrades, contradicting per-line output for unchecked
pins and pins newer than the set. The writer now tracks behind /
ahead / unchecked separately and reports each bucket explicitly.
Codex P2 (stale help): the audit module doc and usage text still
recommended lifting the inline `components` wrapper — the exact unsafe
edit this PR removes. Both now state the wrapper is canonical.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/audit.zig (1)
661-688: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
legacy_file_object_no_rootdoesn't recognizecomponents/overridesas a root-entity marker — will perpetually flag files migrate now correctly refuses to touch.
inspectFileonly checksisPascalCaseKeyto decide "no real root entity" (line 677). Buttransforms_meta.zig'sisEntityShapeKeywas updated in this same PR to treatcomponents/overridesas entity-shape too, specifically so the migrator's directive-to-meta and file-to-array passes never touch a file like:{ "children": [...], "components": { "rooms__Room": { ... } } }This exact shape (a root entity that owns pack-namespaced components via the wrapper, plus its own
children) is precisely the case this PR set out to protect end-to-end — one of the linked-issue objectives is to stopaudit unification/migrate unifiedfrom flagging or lifting "rootcomponents" wrappers. Since audit'slegacy_file_object_no_rootcheck wasn't updated to match, it will report this file as "no root entity — should become a top-level array" (line 683), butmigrate/transforms_meta.zig'sshouldCollapseFileToArray/moveOneDirectiveToMetawill never collapse it (both correctly refuse oncecomponents/overridesis present). The audit finding becomes permanent and unfixable bymigrate unified, breaking the 1:1audit.legacy_file_object_no_root ↔ summary.file_as_array_collapsescontract documented inmigrate/pipeline.zig.🐛 Proposed fix
if (has_children or has_entities_key) { - var any_pascal = false; + var any_pascal = false; var it = file_obj.iterator(); while (it.next()) |kv| { - if (isPascalCaseKey(kv.key_ptr.*)) { + const k = kv.key_ptr.*; + if (isPascalCaseKey(k) or std.mem.eql(u8, k, "components") or std.mem.eql(u8, k, "overrides")) { any_pascal = true; break; } }Consider adding a test mirroring
tests_rfc596.zig'sInlineComponentsPreservedSpec"pack-namespaced (lowercase) component keys stay inside the wrapper" case, assertinglegacy_file_object_no_rootdoes NOT fire for{children, components: {...lowercase...}}.🤖 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/cli/audit.zig` around lines 661 - 688, Update the legacy_file_object_no_root detection in inspectFile to treat the components and overrides keys as root-entity markers alongside PascalCase keys, so wrappers containing either key are not reported as missing a root entity. Keep the existing children/entities gating and reporting behavior unchanged, and add coverage matching the InlineComponentsPreservedSpec shape to verify no finding is emitted.
🧹 Nitpick comments (1)
src/cli/audit.zig (1)
24-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale narrative docs still describe the removed inline-
components-wrapper lift (cli#338) as active. The code sites closest to the removal (audit.zig's 3f struct-removal note at L249-254,pipeline.zig's pass-6 block at L293-301) were correctly updated to explain the removal, but these three higher-level descriptions were left unchanged and now contradict the adjacent code.
src/cli/audit.zig#L24-L53: drop or rewrite item "f" ("components"wrapper on an inline entity...) in the module-doc enumeration of legacy patterns — it's no longer a legacy pattern per cli#338.src/cli/audit.zig#L71-L87: remove"components" wrapper on inline entityfrom theusagehelp text's list of flagged legacy patterns (this text is shown to CLI users via--help).src/cli/migrate/pipeline.zig#L172-L204: update/remove pass "6" in thetransformBytespass-order docblock (currently still says "RFC#596: lift inlinecomponentsblock") to match the "REMOVED (cli#338)" note already present in the pass-6 code block below it.🤖 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/cli/audit.zig` around lines 24 - 53, Remove or rewrite the obsolete inline-"components" wrapper pattern from the module-doc enumeration in src/cli/audit.zig lines 24-53 and the CLI usage help text in src/cli/audit.zig lines 71-87. Update the pass-order documentation for transformBytes in src/cli/migrate/pipeline.zig lines 172-204 to remove or mark pass 6 as removed, matching the existing "REMOVED (cli#338)" note in the pass-6 code block; no code behavior changes are needed.
🤖 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/cli/update_check.zig`:
- Around line 187-194: Update the status wording in the updates == 0 branch of
the update-check flow so it does not claim all pins match when unchecked pins
were excluded from updates. Use wording that explicitly limits the claim to
comparable pins, while preserving the existing updates > 0 message and
compatibility-set note.
---
Outside diff comments:
In `@src/cli/audit.zig`:
- Around line 661-688: Update the legacy_file_object_no_root detection in
inspectFile to treat the components and overrides keys as root-entity markers
alongside PascalCase keys, so wrappers containing either key are not reported as
missing a root entity. Keep the existing children/entities gating and reporting
behavior unchanged, and add coverage matching the InlineComponentsPreservedSpec
shape to verify no finding is emitted.
---
Nitpick comments:
In `@src/cli/audit.zig`:
- Around line 24-53: Remove or rewrite the obsolete inline-"components" wrapper
pattern from the module-doc enumeration in src/cli/audit.zig lines 24-53 and the
CLI usage help text in src/cli/audit.zig lines 71-87. Update the pass-order
documentation for transformBytes in src/cli/migrate/pipeline.zig lines 172-204
to remove or mark pass 6 as removed, matching the existing "REMOVED (cli#338)"
note in the pass-6 code block; no code behavior changes are needed.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b5581a34-ecdd-4172-abde-69cc2abd97d3
📒 Files selected for processing (11)
src/cli/audit.zigsrc/cli/migrate.zigsrc/cli/migrate/pipeline.zigsrc/cli/migrate/tests_rfc596.zigsrc/cli/migrate/transforms.zigsrc/cli/migrate/transforms_meta.zigsrc/cli/migrate/walk.zigsrc/cli/project_config.zigsrc/cli/update_check.zigsrc/cli/upgrade.zigversions.zon
…from the match claim CodeRabbit on #339, same root as codex's summary-honesty P2: a project whose only reportable pin is unchecked would otherwise read as a blanket match. Unchecked pins already get their own explicit note.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/update_check.zig (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the bgfx-compatible-set exception consistently.
The new check path tracks bgfx via
project_config.BGFX_VERSION, but both comments still describe bgfx/backend packages as wholly untracked.
src/cli/update_check.zig#L42-L45: describeerr_backend_untrackedas applying to non-bgfx backend packages.src/cli/upgrade.zig#L189-L195: document bgfx as bundled/compared and other backend packages as unchecked.🤖 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/cli/update_check.zig` around lines 42 - 45, Update the documentation for err_backend_untracked in src/cli/update_check.zig lines 42-45 to state that it applies only to non-bgfx backend packages, while preserving the existing message. In src/cli/upgrade.zig lines 189-195, document that bgfx is bundled and compared via the compatible set, while other backend packages remain unchecked.
🤖 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/cli/upgrade.zig`:
- Around line 296-312: The replaceBackendVersion function must identify the
actual .backend_package block and tokenize its fields before replacing .version,
rather than relying on an exact substring search. Ignore commented-out version
entries and accept valid ZON whitespace variants such as `.version="..."`;
preserve the existing allocator ownership behavior and add regressions covering
both comment and whitespace cases.
---
Nitpick comments:
In `@src/cli/update_check.zig`:
- Around line 42-45: Update the documentation for err_backend_untracked in
src/cli/update_check.zig lines 42-45 to state that it applies only to non-bgfx
backend packages, while preserving the existing message. In src/cli/upgrade.zig
lines 189-195, document that bgfx is bundled and compared via the compatible
set, while other backend packages remain unchecked.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 835bbc45-5a7f-49d4-b036-afaaa7a2b2b4
📒 Files selected for processing (6)
build.zigsrc/cli/audit.zigsrc/cli/project_config.zigsrc/cli/update_check.zigsrc/cli/upgrade.zigversions.zon
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/project_config.zig
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/update_check.zig (1)
175-216: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for human pin summaries.
The new behind/unchecked/ahead branches and wording are not covered by tests. Add cases for only unchecked pins, ahead pins, and mixed statuses to prevent regressions in the
upgrade --checkreport.🤖 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/cli/update_check.zig` around lines 175 - 216, Add regression tests for the upgrade --check human-readable summary covering only unchecked pins, only ahead pins, and a mixed combination of behind, unchecked, and ahead statuses. Assert the branch-specific lines, counts, and summary/note wording produced by the package summary loop and its behind/ahead/unchecked counters, while preserving existing output expectations.
🤖 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/cli/update_check.zig`:
- Around line 175-216: Add regression tests for the upgrade --check
human-readable summary covering only unchecked pins, only ahead pins, and a
mixed combination of behind, unchecked, and ahead statuses. Assert the
branch-specific lines, counts, and summary/note wording produced by the package
summary loop and its behind/ahead/unchecked counters, while preserving existing
output expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8240e1cc-5780-4dba-955e-2c3acd3a8b13
📒 Files selected for processing (1)
src/cli/update_check.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20273be5c4
ℹ️ 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, bp.name, "bgfx") and bp.version.len > 0 and !bp.isLocal()) { | ||
| const bgfx_target = pickTarget("backend_package.version", bp.version, project_config.BGFX_VERSION, force); | ||
| content = try replaceBackendVersion(allocator, content, bp.version, bgfx_target); |
There was a problem hiding this comment.
For a project using an older local bgfx checkout (repo = "local:..." or @...), this condition skips the backend update only after gfx_version has already been rewritten above. The change itself establishes that gfx 1.28.5 and bgfx 0.13.5 must move together, so upgrade all can still leave a local backend on the old contract and produce a project that no longer builds; either keep gfx unchanged for local backends or stop and require a coordinated manual upgrade.
Useful? React with 👍 / 👎.
| if (std.mem.eql(u8, bp.name, "bgfx")) { | ||
| try packages.append(allocator, update_check.packageStatus(bp.name, pinned, project_config.BGFX_VERSION)); |
There was a problem hiding this comment.
When backend_package.name is bgfx but its repo is a local override, this branch bypasses bp.isLocal() and compares the ordinary version field against BGFX_VERSION. Consequently upgrade --check can report the backend as behind, emit exit code 2, and suggest an upgrade even though upgrade all explicitly refuses to update that local backend; handle local bgfx before the name-specific compatible-set comparison.
Useful? React with 👍 / 👎.
| const anchor = std.mem.indexOf(u8, old_content, ".backend_package") orelse return old_content; | ||
| const search = try std.fmt.allocPrint(allocator, ".version = \"{s}\"", .{old_value}); | ||
| defer allocator.free(search); | ||
| const rel = std.mem.indexOf(u8, old_content[anchor..], search) orelse return old_content; | ||
| const idx = anchor + rel; |
There was a problem hiding this comment.
For valid hand-formatted ZON such as .version="0.13.3" in backend_package, this exact spaced search misses the backend field and continues through the rest of the file. If a later plugin uses the conventional .version = "0.13.3" spelling, upgrade all rewrites that plugin instead while leaving bgfx incompatible with the new gfx pin; locate the backend struct's value boundaries and search only within them rather than treating everything after the anchor as backend content.
Useful? React with 👍 / 👎.
| const k_end = findStringEnd(src, k_start); | ||
| const key = src[k_start + 1 .. k_end - 1]; | ||
| if (key.len == 0 or key[0] < 'A' or key[0] > 'Z') return false; |
There was a problem hiding this comment.
When a valid JSONC component key uses a leading escape, such as "\u0050osition", the audit parses it to Position and reports the overrides wrapper as liftable, but this byte-level check sees the raw leading backslash and refuses the migration. That breaks the stated audit-to-migrate count mapping and leaves migrate unified unable to clear its own finding; classify the decoded key consistently with the parsed audit representation.
Useful? React with 👍 / 👎.
…ounded rewrite, escaped-key parity Codex P1: with a LOCAL bgfx checkout, `upgrade all` skipped the backend bump but had already rewritten gfx — recreating the exact broken gfx/bgfx pairing the previous commit guards against. gfx now stays at the current pin for local-bgfx projects (with a loud pointer to the coordinated manual upgrade); `--force` overrides for a checkout that already satisfies the newer contract. No warning when gfx would not have moved anyway. Codex P2: `--check` compared a local bgfx's version field against the set (its own isLocal() only inspects the VERSION string, but the backend's override lives in `repo`) — it could report "behind" / exit 2 for a pin `upgrade all` refuses to touch. Local backends are now unchecked first, regardless of name. Codex P2: the backend `.version` rewrite searched from the anchor to EOF with an exact-spacing probe, so hand-formatted `.version="x"` in the backend struct fell through and a later plugin's conventionally spaced pin could be rewritten instead. The matcher is now bounded to the backend struct's braces and whitespace-tolerant, and returns null (caller warns, content untouched) when the field can't be located. Codex P2: the PascalCase lift guard read raw key bytes while the audit classifies decoded keys, so `"Position"` was audited as liftable but never lifted. The guard now decodes a leading \uXXXX (the only JSON escape that can encode A–Z) before deciding. Regression tests for all four (513/517 green).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e11ad4808
ℹ️ 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".
| } else { | ||
| std.debug.print("labelle: warning: could not locate backend_package .version in project.labelle — bgfx pin NOT updated; set it to {s} manually (paired with gfx {s})\n", .{ bgfx_target, gfx_target }); |
There was a problem hiding this comment.
When a remote bgfx pin uses valid ZON formatting the new matcher does not handle, such as a newline or comment between .version and =, replaceBackendVersion returns null here after gfx_version has already been rewritten. The command then still writes the file and exits successfully, leaving the incompatible gfx/bgfx pairing this change is intended to prevent. The fresh evidence beyond the earlier search-scope issue is this new explicit failure branch: it warns but does not roll back gfx or abort the write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/migrate/transforms.zig (1)
613-620: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard wrapper splicing against mixed-line formatting and lock it with a regression test.
A wrapper sharing its line with
"prefab"can causeliftWrapperAtto start deletion at the line start, removing the preceding sibling. The current cli#337 test does not exercise this path because inlinecomponentsare now preserved without splicing.
src/cli/migrate/transforms.zig#L613-L620: treat the wrapper as line-based only when the bytes before its key on that line are whitespace.src/cli/migrate/tests_rfc596.zig#L170-L189: add a collapsed-closer fixture with an eligible wrapper sharing a line withprefab, and assert the prefab remains after migration.🤖 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/cli/migrate/transforms.zig` around lines 613 - 620, The wrapper-splicing guard in liftWrapperAt must only allow line-based lifting when the bytes preceding the wrapper key on that line are whitespace; update the condition around innerKeysAllPascal accordingly. In src/cli/migrate/transforms.zig lines 613-620, apply this guard; in src/cli/migrate/tests_rfc596.zig lines 170-189, add a collapsed-closer fixture with an eligible wrapper sharing a line with prefab and assert prefab remains after migration.
🧹 Nitpick comments (1)
src/cli/migrate/tests_rfc596.zig (1)
170-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this regression exercise
spliceDropEntry.Because Transform F is removed, this fixture only verifies that inline
componentsremain untouched; it does not cover the collapsed-closer deletion path. Add a case where an eligible emptyoverrideswrapper shares a line withprefaband the outer}is collapsed, then assert that the prefab and parsed structure survive.🤖 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/cli/migrate/tests_rfc596.zig` around lines 170 - 189, The regression test around applyAllArenaFull currently does not exercise spliceDropEntry. Extend the fixture with an eligible empty overrides wrapper sharing a line with prefab and a collapsed outer closing brace, then assert the prefab remains present and the output still parses as the expected object structure.
🤖 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/cli/upgrade.zig`:
- Around line 167-172: Update the upgrade flow around replaceBackendVersion and
the earlier gfx_version rewrite so the command never writes a split gfx/bgfx
upgrade. Preflight the backend replacement before applying gfx, or abort without
writing whenever the backend replacement returns null; preserve the existing
paired-version behavior on success and add an end-to-end regression covering the
failed backend update.
---
Outside diff comments:
In `@src/cli/migrate/transforms.zig`:
- Around line 613-620: The wrapper-splicing guard in liftWrapperAt must only
allow line-based lifting when the bytes preceding the wrapper key on that line
are whitespace; update the condition around innerKeysAllPascal accordingly. In
src/cli/migrate/transforms.zig lines 613-620, apply this guard; in
src/cli/migrate/tests_rfc596.zig lines 170-189, add a collapsed-closer fixture
with an eligible wrapper sharing a line with prefab and assert prefab remains
after migration.
---
Nitpick comments:
In `@src/cli/migrate/tests_rfc596.zig`:
- Around line 170-189: The regression test around applyAllArenaFull currently
does not exercise spliceDropEntry. Extend the fixture with an eligible empty
overrides wrapper sharing a line with prefab and a collapsed outer closing
brace, then assert the prefab remains present and the output still parses as the
expected object structure.
🪄 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: CHILL
Plan: Pro Plus
Run ID: d6d8f1c2-5fcc-4c32-93c5-8b6839230a0a
📒 Files selected for processing (3)
src/cli/migrate/tests_rfc596.zigsrc/cli/migrate/transforms.zigsrc/cli/upgrade.zig
…dex round 3) The round-2 failure branch warned when the bgfx pin could not be located (valid-but-exotic ZON, e.g. a newline between `.version` and `=`) — but gfx had already been rewritten and the file was still written, producing the incompatible gfx/bgfx pairing this guard exists to prevent. The backend bump now runs FIRST and its outcome gates gfx: - local bgfx checkout → gfx kept (round 2 behavior) - pinned bgfx, rewrite fails → gfx kept, loud warning, coherent file - pinned bgfx, rewritten → gfx moves with it - no / non-bgfx backend → gfx moves (nothing to pair with) `--force` overrides the gate in all cases. Verified end-to-end on a fixture with `.version\n= "0.13.3"`: bgfx left at 0.13.3, gfx kept at 1.26.2, core/engine still upgraded, warnings name the manual step. 513/517 tests green.
2 participants
Closes #336, closes #337, closes #338.
Found while migrating flying-platform-labelle to engine 2.x — all three tools misled or failed during that upgrade.
Commit 1 — migrate/audit (#337, #338)
#338 — the inline
components:wrapper is not legacy. The engine's case-convention rule (RFC #596) only reads PascalCase flat keys as components; pack-namespaced lowercase keys (rooms__Room) are only recognized inside the wrapper. Lifting them flat made the engine silently drop the components. Engine v2.0 removedcomponentson prefab references only.isEntityShapeKeytreatscomponents/overridesas entity content, so the directives-to-meta pass can't relocate a root entity's components intometa:(the wrapper only survives to that pass since this change).#337 — project-wide
UnexpectedEndOfInputabort. Chain: unsafe lift exposes a lowercase key → directive mover's line-based splice eats the file's closing brace when the value ends against collapsed closers (}}) → next pass re-parses the corrupted buffer → error propagates project-wide with no file name.deleteTopLevelKey/spliceDropEntryonly extend a cut to end-of-line when the line tail is blank/comment.transformBytesfailures contained per file (path + error reported, file left unmodified, run continues).Commit 2 — upgrade --check (#336)
versions.zonrefreshed to core 1.27.0 / engine 2.10.0 / gfx 1.28.5 — verified together on flying-platform-labelle (clean build from pristine caches, 227-file suite green, 3 scenes run without panics). The old set recommended engine 2.5.0, which cannot compile any project with a large pack (fixed upstream in 2.10.0, engine#795).backend_package(bgfx) now parsed and reported with an explicit "latest not tracked" note instead of being silently omitted — its omission hid the coordinated gfx+bgfx bump the backend-contract boundary requires.Verification
zig build test: 509/513 passed (4 skipped, 0 failed)tests_rfc596.zig/audit.zigspecs.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.