refactor(cli): split migrate.zig into migrate/ submodules (3514 → 248) - #268
Conversation
Pure mechanical move of the 3514-line migrate.zig into cohesive submodules under src/cli/migrate/, each under 1000 lines. Function bodies are byte-identical to the original; the only edits are adding pub/const/@import wiring and module qualifiers on call sites. migrate.zig (248) — CLI dispatch + run orchestration migrate/scanner.zig (265) — JSONC byte scanner + JSONC→JSON stripper migrate/transforms.zig (768) — byte transforms A-F migrate/transforms_meta.zig (498) — RFC #596 meta/directive transforms G-I migrate/pipeline.zig (401) — transform dispatcher + Summary/FileCounts migrate/walk.zig (264) — directory traversal + xref pre-scan migrate/tests*.zig — zspec spec namespaces (verbatim) zig build clean; zig build test passes (286/286, unchanged).
📝 WalkthroughWalkthroughAdds a complete JSONC migration pipeline under ChangesJSONC Migration Pipeline
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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 introduces a byte-oriented transform pipeline and counts model for migrating JSONC files, implementing directory walking, a JSONC-aware scanner, and various structural transforms. The review feedback is highly valuable and identifies several critical issues: potential runtime panics from slicing unclosed strings in findTopLevelKey, objectHasPrefabStringAndComponents, and objectHasWrapper; a bug in spliceDropEntry where the backward walk does not skip comments when searching for a preceding comma; and a memory leak in walkAndMigrate due to allocating temporary file buffers in the long-lived main arena instead of a per-file reset temporary arena.
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.
| const k_start = i; | ||
| const k_end = findStringEnd(src, k_start); | ||
| const this_key = src[k_start + 1 .. k_end - 1]; |
There was a problem hiding this comment.
If the file ends with an unclosed string at the top level, findStringEnd will return src.len. This causes k_end - 1 to be less than k_start + 1, leading to a runtime panic due to an invalid slice range (slice start index larger than end index). Adding a check to ensure the string is properly closed before slicing prevents this panic.
const k_start = i;
const k_end = findStringEnd(src, k_start);
if (k_end == src.len and src[src.len - 1] != '"') return null;
const this_key = src[k_start + 1 .. k_end - 1];
| } | ||
| const k_start = i; | ||
| const k_end = findStringEnd(src, k_start); |
There was a problem hiding this comment.
If the file contains an unclosed string inside an object, findStringEnd will return src.len. This causes k_end - 1 to be less than k_start + 1, leading to a runtime panic when slicing key. Adding a check to ensure the string is properly closed before slicing prevents this panic.
const k_start = i;
const k_end = findStringEnd(src, k_start);
if (k_end == src.len and src[src.len - 1] != '"') return null;
const key = src[k_start + 1 .. k_end - 1];
| const k_start = i; | ||
| const k_end = findStringEnd(src, k_start); | ||
| const key = src[k_start + 1 .. k_end - 1]; |
There was a problem hiding this comment.
If the file contains an unclosed string inside an object, findStringEnd will return src.len. This causes k_end - 1 to be less than k_start + 1, leading to a runtime panic when slicing key. Adding a check to ensure the string is properly closed before slicing prevents this panic.
const k_start = i;
const k_end = findStringEnd(src, k_start);
if (k_end == src.len and src[src.len - 1] != '"') return null;
const key = src[k_start + 1 .. k_end - 1];
| var p: isize = @intCast(cut_start); | ||
| p -= 1; | ||
| while (p >= 0) : (p -= 1) { | ||
| const c = src[@intCast(p)]; | ||
| if (c == ' ' or c == '\t' or c == '\r' or c == '\n') continue; | ||
| if (c == ',') { | ||
| cut_start = @intCast(p); | ||
| break; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Unlike deleteTopLevelKey, spliceDropEntry's backward walk does not skip comments when searching for the preceding comma. If there are comments between the preceding comma and the dropped entry, the walk will break early and fail to remove the comma, resulting in invalid JSON. Reusing the comment-skipping logic from deleteTopLevelKey makes this robust.
var p: isize = @intCast(cut_start);
p -= 1;
while (p >= 0) : (p -= 1) {
const c = src[@intCast(p)];
if (c == ' ' or c == '\t' or c == '\r') continue;
if (c == '\n') {
const newline_idx: usize = @intCast(p);
var ls: usize = newline_idx;
while (ls > 0 and src[ls - 1] != '\n') ls -= 1;
var s: usize = ls;
while (s < newline_idx and (src[s] == ' ' or src[s] == '\t')) s += 1;
if (s + 1 < newline_idx and src[s] == '/' and src[s + 1] == '/') {
p = @as(isize, @intCast(ls));
}
continue;
}
if (c == '/' and p > 0 and src[@intCast(p - 1)] == '*') {
p -= 2;
while (p >= 1 and !(src[@intCast(p - 1)] == '/' and src[@intCast(p)] == '*')) p -= 1;
if (p >= 1) p -= 1;
continue;
}
if (c == ',') {
cut_start = @intCast(p);
break;
}
break;
}
| pub fn walkAndMigrate( | ||
| arena: std.mem.Allocator, | ||
| project_dir: []const u8, | ||
| subdir: []const u8, | ||
| dry_run: bool, | ||
| xrefs: *const std.StringHashMap(void), | ||
| summary: *Summary, | ||
| ) !void { | ||
| const io = config.globalIo(); | ||
| const full = try std.fs.path.join(arena, &.{ project_dir, subdir }); | ||
| var dir = std.Io.Dir.cwd().openDir(io, full, .{ .iterate = true }) catch |err| switch (err) { | ||
| // A missing scenes/ or prefabs/ is fine — tiny fixtures often | ||
| // have only one of the two. The migrator just contributes zero | ||
| // findings for that subdir; no error. | ||
| error.FileNotFound => return, | ||
| else => return err, | ||
| }; | ||
| defer dir.close(io); | ||
|
|
||
| var rel_buf: std.ArrayList(u8) = .empty; | ||
| try rel_buf.appendSlice(arena, subdir); | ||
| try walkSubdir(arena, &dir, full, &rel_buf, dry_run, xrefs, summary); | ||
| } | ||
|
|
||
| fn walkSubdir( | ||
| arena: std.mem.Allocator, | ||
| dir: *std.Io.Dir, | ||
| abs_dir: []const u8, | ||
| rel_buf: *std.ArrayList(u8), | ||
| dry_run: bool, | ||
| xrefs: *const std.StringHashMap(void), | ||
| summary: *Summary, | ||
| ) !void { | ||
| const io = config.globalIo(); | ||
| var iter = dir.iterate(); | ||
| while (try iter.next(io)) |entry| { | ||
| const saved_rel = rel_buf.items.len; | ||
| defer rel_buf.shrinkRetainingCapacity(saved_rel); | ||
| try rel_buf.append(arena, std.fs.path.sep); | ||
| try rel_buf.appendSlice(arena, entry.name); | ||
|
|
||
| switch (entry.kind) { | ||
| .directory => { | ||
| var sub = try dir.openDir(io, entry.name, .{ .iterate = true }); | ||
| defer sub.close(io); | ||
| const sub_abs = try std.fs.path.join(arena, &.{ abs_dir, entry.name }); | ||
| try walkSubdir(arena, &sub, sub_abs, rel_buf, dry_run, xrefs, summary); | ||
| }, | ||
| .file => { | ||
| if (!std.mem.endsWith(u8, entry.name, ".jsonc")) continue; | ||
| try migrateFile(arena, dir, entry.name, rel_buf.items, dry_run, xrefs, summary); | ||
| }, | ||
| else => {}, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
While collectPrefabRefs uses a temporary arena allocator and resets it after each file to keep memory usage low, walkAndMigrate allocates all file buffers, stripped JSON buffers, parsed JSON trees, and intermediate transformed buffers in the long-lived main arena. For projects with many files, this results in a significant memory leak that persists until the command exits. Introducing a temp_arena and resetting it after each file (matching the pattern in collectPrefabRefs) keeps memory usage extremely low and constant.
pub fn walkAndMigrate(
arena: std.mem.Allocator,
project_dir: []const u8,
subdir: []const u8,
dry_run: bool,
xrefs: *const std.StringHashMap(void),
summary: *Summary,
) !void {
const io = config.globalIo();
const full = try std.fs.path.join(arena, &.{ project_dir, subdir });
var dir = std.Io.Dir.cwd().openDir(io, full, .{ .iterate = true }) catch |err| switch (err) {
// A missing scenes/ or prefabs/ is fine — tiny fixtures often
// have only one of the two. The migrator just contributes zero
// findings for that subdir; no error.
error.FileNotFound => return,
else => return err,
};
defer dir.close(io);
var rel_buf: std.ArrayList(u8) = .empty;
try rel_buf.appendSlice(arena, subdir);
var temp_arena = std.heap.ArenaAllocator.init(arena);
defer temp_arena.deinit();
try walkSubdir(arena, &temp_arena, &dir, full, &rel_buf, dry_run, xrefs, summary);
}
fn walkSubdir(
arena: std.mem.Allocator,
temp_arena: *std.heap.ArenaAllocator,
dir: *std.Io.Dir,
abs_dir: []const u8,
rel_buf: *std.ArrayList(u8),
dry_run: bool,
xrefs: *const std.StringHashMap(void),
summary: *Summary,
) !void {
const io = config.globalIo();
var iter = dir.iterate();
while (try iter.next(io)) |entry| {
const saved_rel = rel_buf.items.len;
defer rel_buf.shrinkRetainingCapacity(saved_rel);
try rel_buf.append(arena, std.fs.path.sep);
try rel_buf.appendSlice(arena, entry.name);
switch (entry.kind) {
.directory => {
var sub = try dir.openDir(io, entry.name, .{ .iterate = true });
defer sub.close(io);
const sub_abs = try std.fs.path.join(arena, &.{ abs_dir, entry.name });
try walkSubdir(arena, temp_arena, &sub, sub_abs, rel_buf, dry_run, xrefs, summary);
},
.file => {
if (!std.mem.endsWith(u8, entry.name, ".jsonc")) continue;
const temp = temp_arena.allocator();
try migrateFile(temp, dir, entry.name, rel_buf.items, dry_run, xrefs, summary);
_ = temp_arena.reset(.retain_capacity);
},
else => {},
}
}
}
|
Safe to merge — the 2 red checks are pre-existing, not from this PR. The 5 Gemini findings (e.g. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/migrate/pipeline.zig`:
- Line 264: The `catch null` handling in `transformBytes` is masking real
failures from helper transforms and can leave migrations partially applied while
still succeeding. Update the affected edit paths in `transformBytes` to
propagate errors from `transforms.renameOneComponentsOnRef`, and the other
similar helper calls at the referenced sites, instead of converting them to
null/no-op results. Keep the error union flowing through these operations so
allocation or transform failures bubble up to the caller.
In `@src/cli/migrate/scanner.zig`:
- Around line 36-38: The `Scanner`’s `line_start` handling is letting top-level
entries point before the current entry when a key appears on the same line as
`{`, which can make callers splice out the opening brace or earlier siblings.
Update the logic in the scanner routine that computes `line_start` so it always
stays within the current entry for compact JSONC, and verify the behavior used
by `line_start` consumers remains anchored to the entry itself.
In `@src/cli/migrate/transforms_meta.zig`:
- Around line 239-264: The insertion logic in transforms_meta.zig currently
appends the new meta entry at splice_at without accounting for a trailing line
comment, so a key added after a `//` comment can be swallowed by that comment.
Update the insertion point logic around the existing whitespace scan and
has_trailing_comma handling to detect a trailing `//` comment before the closing
brace and place the separator/key on a new line outside the comment, preserving
the existing comment and the moved directive.
- Around line 40-50: The moveNameToMeta function is dropping the top-level name
whenever has_meta is true instead of preserving it. Update moveNameToMeta so
that, when a meta block already exists, it merges the divergent name into that
existing meta content rather than calling deleteTopLevelKey on "name"; use the
existing moveNameToMeta path and its helpers to keep both fields intact.
In `@src/cli/migrate/transforms.zig`:
- Around line 461-470: `treeHasInlineComponentsWrapper` is stricter than the
parsed sibling check used by `objectHasWrapper`, since it rejects any object
with a `prefab` key even when the value is null. Update the raw matcher to
mirror the parsed “no prefab sibling” logic so it only treats a prefab sibling
as disqualifying when it matches the same condition as
`objectHasWrapper`/`require_sibling_prefab`, and keep the inline `components`
detection consistent with that rule. Apply the same adjustment anywhere the pass
6 lifting logic relies on this matcher so objects with `"prefab": null` are not
misclassified.
- Around line 225-228: The comma reinsertion in the lifted-entry handling is
being appended inside the copied body, which lets a trailing line comment
swallow the separator and can also emit a stray comma for an empty lifted root
body. Update the logic around the lifted body handling that uses
loc.comma_after, splice_end, and dedented.append so the separator is emitted
outside any trailing // comment and skipped entirely when the lifted body is
empty, keeping the next sibling separated correctly.
In `@src/cli/migrate/walk.zig`:
- Around line 75-79: The pre-scan in walk.zig is swallowing file read and prefab
scan failures, which leaves TransformCtx.xrefs incomplete without any signal.
Update the logic around dir.readFileAlloc and scanPrefabRefs to record or log
these failures instead of using empty catches, and thread the failure
count/status into Summary so callers can tell the xref set is partial. Keep the
fix localized near the pre-scan loop that populates xrefs and preserve the
temp_arena reset behavior for failed reads.
- Around line 210-238: migrateFile is still doing the second-pass work in the
shared long-lived arena, so per-file allocations for raw, stripped, the parsed
JSON tree, and transformBytes outputs accumulate across the whole run. Move the
second-pass allocations to a temporary per-file arena that is
reset/deinitialized after each file, while keeping only the needed write-out
result and counters. Make sure the per-file arena is used around
dir.readFileAlloc, stripJsoncToJson, std.json.parseFromSlice, and transformBytes
so each file’s memory is released before processing the next one.
🪄 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: bb87cfc4-7334-4c94-8a25-4be89b844c17
📒 Files selected for processing (9)
src/cli/migrate.zigsrc/cli/migrate/pipeline.zigsrc/cli/migrate/scanner.zigsrc/cli/migrate/tests.zigsrc/cli/migrate/tests_helpers.zigsrc/cli/migrate/tests_rfc596.zigsrc/cli/migrate/transforms.zigsrc/cli/migrate/transforms_meta.zigsrc/cli/migrate/walk.zig
| const stripped = try stripJsoncToJson(arena, current); | ||
| var parsed = try std.json.parseFromSlice(std.json.Value, arena, stripped, .{}); | ||
| defer parsed.deinit(); | ||
| const edited = transforms.renameOneComponentsOnRef(arena, current, parsed.value) catch null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate transform helper errors instead of treating them as no-ops.
These catch null sites silently skip failed edits, which can produce partially migrated output while reporting success. Since transformBytes already returns an error union, let allocation/transform errors bubble up.
Proposed fix
- const edited = transforms.renameOneComponentsOnRef(arena, current, parsed.value) catch null;
+ const edited = try transforms.renameOneComponentsOnRef(arena, current, parsed.value);
@@
- const edited = transforms.liftOneOverridesBlock(arena, current, parsed.value) catch null;
+ const edited = try transforms.liftOneOverridesBlock(arena, current, parsed.value);
@@
- const edited = transforms.liftOneComponentsBlock(arena, current, parsed.value) catch null;
+ const edited = try transforms.liftOneComponentsBlock(arena, current, parsed.value);
@@
- const edited = transforms_meta.moveOneDirectiveToMeta(arena, current, parsed.value) catch null;
+ const edited = try transforms_meta.moveOneDirectiveToMeta(arena, current, parsed.value);Also applies to: 282-282, 299-299, 361-361
🤖 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/pipeline.zig` at line 264, The `catch null` handling in
`transformBytes` is masking real failures from helper transforms and can leave
migrations partially applied while still succeeding. Update the affected edit
paths in `transformBytes` to propagate errors from
`transforms.renameOneComponentsOnRef`, and the other similar helper calls at the
referenced sites, instead of converting them to null/no-op results. Keep the
error union flowing through these operations so allocation or transform failures
bubble up to the caller.
| /// Index of the start-of-line of the entry (the previous `\n`+1, | ||
| /// or the `{`+1 if this is the first sibling). | ||
| line_start: usize, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep line_start inside the current entry.
Line 78 walks to byte 0 when a top-level key is on the same line as {, so callers that splice from loc.line_start can delete the opening brace or earlier siblings for compact JSONC like { "assets": {}, "name": "x" }.
Proposed fix
pub fn findTopLevelKey(src: []const u8, key: []const u8) ?KeyLoc {
// Skip whitespace/comments to find the opening `{`.
var i: usize = skipWsAndComments(src, 0);
if (i >= src.len or src[i] != '{') return null;
i += 1;
+ var entry_start = i;
while (true) {
+ entry_start = i;
i = skipWsAndComments(src, i);
if (i >= src.len) return null;
if (src[i] == '}') return null;
if (src[i] != '"') return null; // malformed; bail
@@
if (std.mem.eql(u8, this_key, key)) {
- // Find line_start: walk back to the previous '\n'+1.
+ // Find line_start: walk back to the previous '\n'+1, but
+ // never before this entry's start within the containing object.
var ls: usize = k_start;
- while (ls > 0 and src[ls - 1] != '\n') ls -= 1;
+ while (ls > entry_start and src[ls - 1] != '\n') ls -= 1;
return KeyLoc{Also applies to: 76-86
🤖 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/scanner.zig` around lines 36 - 38, The `Scanner`’s
`line_start` handling is letting top-level entries point before the current
entry when a key appears on the same line as `{`, which can make callers splice
out the opening brace or earlier siblings. Update the logic in the scanner
routine that computes `line_start` so it always stays within the current entry
for compact JSONC, and verify the behavior used by `line_start` consumers
remains anchored to the entry itself.
| pub fn moveNameToMeta(arena: std.mem.Allocator, src: []const u8, name_value: []const u8, has_meta: bool) ?[]u8 { | ||
| _ = name_value; | ||
| if (has_meta) { | ||
| // Conservative: leave the existing `meta:` block alone (merging | ||
| // is structurally risky to do byte-level). Just drop the bare | ||
| // `name:` so the audit only re-fires for divergent-name files | ||
| // that actually NEED human attention. This case is also rare | ||
| // enough across FP / bouncing-ball that the simpler behaviour | ||
| // is preferable to a half-correct merge. If/when we see a real | ||
| // case in the smoke run we'll extend this. | ||
| return deleteTopLevelKey(arena, src, "name"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve divergent name when meta already exists.
Line 50 deletes the top-level name instead of moving it into the existing meta block, so a file with both name and meta loses the divergent name during migration.
Proposed fix
pub fn moveNameToMeta(arena: std.mem.Allocator, src: []const u8, name_value: []const u8, has_meta: bool) ?[]u8 {
_ = name_value;
if (has_meta) {
- // Conservative: leave the existing `meta:` block alone (merging
- // is structurally risky to do byte-level). Just drop the bare
- // `name:` so the audit only re-fires for divergent-name files
- // that actually NEED human attention. This case is also rare
- // enough across FP / bouncing-ball that the simpler behaviour
- // is preferable to a half-correct merge. If/when we see a real
- // case in the smoke run we'll extend this.
- return deleteTopLevelKey(arena, src, "name");
+ const loc = findTopLevelKey(src, "name") orelse return null;
+ if (loc.value_start >= src.len or src[loc.value_start] != '"') return null;
+ const value_bytes = src[loc.value_start..loc.value_end];
+ const without_name = deleteTopLevelKey(arena, src, "name") orelse return null;
+ return mergeIntoExistingMeta(arena, without_name, "name", value_bytes) catch null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn moveNameToMeta(arena: std.mem.Allocator, src: []const u8, name_value: []const u8, has_meta: bool) ?[]u8 { | |
| _ = name_value; | |
| if (has_meta) { | |
| // Conservative: leave the existing `meta:` block alone (merging | |
| // is structurally risky to do byte-level). Just drop the bare | |
| // `name:` so the audit only re-fires for divergent-name files | |
| // that actually NEED human attention. This case is also rare | |
| // enough across FP / bouncing-ball that the simpler behaviour | |
| // is preferable to a half-correct merge. If/when we see a real | |
| // case in the smoke run we'll extend this. | |
| return deleteTopLevelKey(arena, src, "name"); | |
| pub fn moveNameToMeta(arena: std.mem.Allocator, src: []const u8, name_value: []const u8, has_meta: bool) ?[]u8 { | |
| _ = name_value; | |
| if (has_meta) { | |
| const loc = findTopLevelKey(src, "name") orelse return null; | |
| if (loc.value_start >= src.len or src[loc.value_start] != '"') return null; | |
| const value_bytes = src[loc.value_start..loc.value_end]; | |
| const without_name = deleteTopLevelKey(arena, src, "name") orelse return null; | |
| return mergeIntoExistingMeta(arena, without_name, "name", value_bytes) catch 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/cli/migrate/transforms_meta.zig` around lines 40 - 50, The moveNameToMeta
function is dropping the top-level name whenever has_meta is true instead of
preserving it. Update moveNameToMeta so that, when a meta block already exists,
it merges the divergent name into that existing meta content rather than calling
deleteTopLevelKey on "name"; use the existing moveNameToMeta path and its
helpers to keep both fields intact.
| while (splice_at > meta_open + 1) { | ||
| const c = src[splice_at - 1]; | ||
| if (c == ' ' or c == '\t' or c == '\n' or c == '\r') { | ||
| splice_at -= 1; | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
| // JSONC allows a trailing comma before `}`. If the last structural | ||
| // byte is already a `,`, we must NOT prepend another `,` — that | ||
| // would yield `..., , "newkey": ...` (invalid JSON). Detect it and | ||
| // suppress the prepended separator. | ||
| const has_trailing_comma = !empty and splice_at > meta_open + 1 and | ||
| src[splice_at - 1] == ','; | ||
| out.appendSlice(arena, src[0..splice_at]) catch return null; | ||
| if (!empty and !has_trailing_comma) { | ||
| out.appendSlice(arena, ", ") catch return null; | ||
| } else { | ||
| out.append(arena, ' ') catch return null; | ||
| } | ||
| out.append(arena, '"') catch return null; | ||
| out.appendSlice(arena, key) catch return null; | ||
| out.appendSlice(arena, "\": ") catch return null; | ||
| out.appendSlice(arena, value_bytes) catch return null; | ||
| out.append(arena, ' ') catch return null; | ||
| out.appendSlice(arena, src[splice_at..]) catch return null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Avoid inserting entries after trailing // comments.
If an existing meta object ends with a line comment before }, Line 254 appends the separator and new key on that same commented line, so the moved directive can be stripped as part of the comment. Use a comment-aware insertion point, or emit the separator/key on a new line outside any trailing // comment.
🤖 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_meta.zig` around lines 239 - 264, The insertion
logic in transforms_meta.zig currently appends the new meta entry at splice_at
without accounting for a trailing line comment, so a key added after a `//`
comment can be swallowed by that comment. Update the insertion point logic
around the existing whitespace scan and has_trailing_comma handling to detect a
trailing `//` comment before the closing brace and place the separator/key on a
new line outside the comment, preserving the existing comment and the moved
directive.
| if (loc.comma_after) |c| { | ||
| splice_end = c + 1; | ||
| dedented.append(arena, ',') catch return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Place reinserted commas outside trailing line comments.
Lines 227 and 708 append the separator after the lifted body. If the lifted entry ends with // comment and the wrapper/root is followed by another sibling, the comma becomes part of the comment, so the next strip/parse pass sees adjacent object entries without a separator. Also avoid emitting a comma when the lifted root body is empty.
Also applies to: 706-709
🤖 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 225 - 228, The comma reinsertion
in the lifted-entry handling is being appended inside the copied body, which
lets a trailing line comment swallow the separator and can also emit a stray
comma for an empty lifted root body. Update the logic around the lifted body
handling that uses loc.comma_after, splice_end, and dedented.append so the
separator is emitted outside any trailing // comment and skipped entirely when
the lifted body is empty, keeping the next sibling separated correctly.
| pub fn treeHasInlineComponentsWrapper(value: std.json.Value) bool { | ||
| switch (value) { | ||
| .object => |obj| { | ||
| const has_prefab = obj.get("prefab") != null; | ||
| if (!has_prefab) { | ||
| if (obj.get("components")) |cv| { | ||
| if (cv == .object) return true; | ||
| } | ||
| } | ||
| var it = obj.iterator(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the raw matcher with the parsed “no prefab sibling” check.
treeHasInlineComponentsWrapper rejects any object with a prefab key, but objectHasWrapper only rejects string-valued prefab siblings when require_sibling_prefab is false. If an earlier object has "prefab": null plus components, and a later object makes the parsed guard true, pass 6 can lift the earlier object unexpectedly.
Proposed fix
pub fn objectHasWrapper(src: []const u8, start_brace: usize, end_one_past: usize, spec: WrapperSpec) ?usize {
var i = start_brace + 1;
+ var has_prefab = false;
var prefab_is_string = false;
var wrapper_pos: ?usize = null;
@@
if (std.mem.eql(u8, key, "prefab")) {
+ has_prefab = true;
if (v_start < src.len and src[v_start] == '"') prefab_is_string = true;
} else if (std.mem.eql(u8, key, spec.wrapper)) {
@@
if (wrapper_pos == null) return null;
if (spec.require_sibling_prefab and !prefab_is_string) return null;
- if (!spec.require_sibling_prefab and prefab_is_string) return null;
+ if (!spec.require_sibling_prefab and has_prefab) return null;
return wrapper_pos;
}Also applies to: 532-570
🤖 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 461 - 470,
`treeHasInlineComponentsWrapper` is stricter than the parsed sibling check used
by `objectHasWrapper`, since it rejects any object with a `prefab` key even when
the value is null. Update the raw matcher to mirror the parsed “no prefab
sibling” logic so it only treats a prefab sibling as disqualifying when it
matches the same condition as `objectHasWrapper`/`require_sibling_prefab`, and
keep the inline `components` detection consistent with that rule. Apply the same
adjustment anywhere the pass 6 lifting logic relies on this matcher so objects
with `"prefab": null` are not misclassified.
| const raw = dir.readFileAlloc(io, entry.name, temp, .limited(1024 * 1024)) catch { | ||
| _ = temp_arena.reset(.retain_capacity); | ||
| continue; | ||
| }; | ||
| scanPrefabRefs(arena, temp, raw, xrefs) catch {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Surface pre-scan failures instead of silently dropping them.
TransformCtx.xrefs is documented as the project-wide set of prefab references, and pass 7 uses it to decide whether to emit divergent-name warnings. Swallowing read/parse failures here means those warnings become incomplete with no signal to the user. At minimum, log/count pre-scan failures; ideally thread them into Summary so callers know the xref set is partial.
🤖 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/walk.zig` around lines 75 - 79, The pre-scan in walk.zig is
swallowing file read and prefab scan failures, which leaves TransformCtx.xrefs
incomplete without any signal. Update the logic around dir.readFileAlloc and
scanPrefabRefs to record or log these failures instead of using empty catches,
and thread the failure count/status into Summary so callers can tell the xref
set is partial. Keep the fix localized near the pre-scan loop that populates
xrefs and preserve the temp_arena reset behavior for failed reads.
| const raw = dir.readFileAlloc(io, entry_name, arena, .limited(1024 * 1024)) catch |err| { | ||
| std.debug.print("labelle migrate unified: could not read '{s}': {s}\n", .{ rel_path, @errorName(err) }); | ||
| summary.parse_errors += 1; | ||
| return; | ||
| }; | ||
|
|
||
| // Parse the JSONC-stripped form once up-front so transforms can | ||
| // consult a structural view (top-level keys, prefab-ref objects) | ||
| // without re-implementing JSONC tokenization. | ||
| const stripped = stripJsoncToJson(arena, raw) catch { | ||
| std.debug.print("labelle migrate unified: could not pre-strip '{s}'\n", .{rel_path}); | ||
| summary.parse_errors += 1; | ||
| return; | ||
| }; | ||
| var parsed = std.json.parseFromSlice(std.json.Value, arena, stripped, .{}) catch |err| { | ||
| std.debug.print("labelle migrate unified: could not parse '{s}': {s}\n", .{ rel_path, @errorName(err) }); | ||
| summary.parse_errors += 1; | ||
| return; | ||
| }; | ||
| defer parsed.deinit(); | ||
|
|
||
| const basename = basenameNoExt(entry_name); | ||
| var counts = FileCounts{}; | ||
| const ctx = TransformCtx{ | ||
| .basename = basename, | ||
| .xrefs = xrefs, | ||
| .rel_path = rel_path, | ||
| }; | ||
| const out = try transformBytes(arena, raw, parsed.value, ctx, &counts); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a per-file arena for the second pass too.
The first pass already resets a temporary arena per file, but migrateFile allocates raw, stripped, the parsed tree, and every intermediate transformBytes buffer in the long-lived arena. On a large project that makes peak memory grow with the sum of all migrated files, even though each file is written and discarded independently.
Suggested direction
fn migrateFile(
arena: std.mem.Allocator,
dir: *std.Io.Dir,
@@
) !void {
const io = config.globalIo();
summary.files_scanned += 1;
+
+ var file_arena = std.heap.ArenaAllocator.init(arena);
+ defer file_arena.deinit();
+ const file_alloc = file_arena.allocator();
- const raw = dir.readFileAlloc(io, entry_name, arena, .limited(1024 * 1024)) catch |err| {
+ const raw = dir.readFileAlloc(io, entry_name, file_alloc, .limited(1024 * 1024)) catch |err| {
std.debug.print("labelle migrate unified: could not read '{s}': {s}\n", .{ rel_path, `@errorName`(err) });
summary.parse_errors += 1;
return;
};
@@
- const stripped = stripJsoncToJson(arena, raw) catch {
+ const stripped = stripJsoncToJson(file_alloc, raw) catch {
std.debug.print("labelle migrate unified: could not pre-strip '{s}'\n", .{rel_path});
summary.parse_errors += 1;
return;
};
- var parsed = std.json.parseFromSlice(std.json.Value, arena, stripped, .{}) catch |err| {
+ var parsed = std.json.parseFromSlice(std.json.Value, file_alloc, stripped, .{}) catch |err| {
std.debug.print("labelle migrate unified: could not parse '{s}': {s}\n", .{ rel_path, `@errorName`(err) });
summary.parse_errors += 1;
return;
};
@@
- const out = try transformBytes(arena, raw, parsed.value, ctx, &counts);
+ const out = try transformBytes(file_alloc, raw, parsed.value, ctx, &counts);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const raw = dir.readFileAlloc(io, entry_name, arena, .limited(1024 * 1024)) catch |err| { | |
| std.debug.print("labelle migrate unified: could not read '{s}': {s}\n", .{ rel_path, @errorName(err) }); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| // Parse the JSONC-stripped form once up-front so transforms can | |
| // consult a structural view (top-level keys, prefab-ref objects) | |
| // without re-implementing JSONC tokenization. | |
| const stripped = stripJsoncToJson(arena, raw) catch { | |
| std.debug.print("labelle migrate unified: could not pre-strip '{s}'\n", .{rel_path}); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| var parsed = std.json.parseFromSlice(std.json.Value, arena, stripped, .{}) catch |err| { | |
| std.debug.print("labelle migrate unified: could not parse '{s}': {s}\n", .{ rel_path, @errorName(err) }); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| defer parsed.deinit(); | |
| const basename = basenameNoExt(entry_name); | |
| var counts = FileCounts{}; | |
| const ctx = TransformCtx{ | |
| .basename = basename, | |
| .xrefs = xrefs, | |
| .rel_path = rel_path, | |
| }; | |
| const out = try transformBytes(arena, raw, parsed.value, ctx, &counts); | |
| const io = config.globalIo(); | |
| summary.files_scanned += 1; | |
| var file_arena = std.heap.ArenaAllocator.init(arena); | |
| defer file_arena.deinit(); | |
| const file_alloc = file_arena.allocator(); | |
| const raw = dir.readFileAlloc(io, entry_name, file_alloc, .limited(1024 * 1024)) catch |err| { | |
| std.debug.print("labelle migrate unified: could not read '{s}': {s}\n", .{ rel_path, `@errorName`(err) }); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| // Parse the JSONC-stripped form once up-front so transforms can | |
| // consult a structural view (top-level keys, prefab-ref objects) | |
| // without re-implementing JSONC tokenization. | |
| const stripped = stripJsoncToJson(file_alloc, raw) catch { | |
| std.debug.print("labelle migrate unified: could not pre-strip '{s}'\n", .{rel_path}); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| var parsed = std.json.parseFromSlice(std.json.Value, file_alloc, stripped, .{}) catch |err| { | |
| std.debug.print("labelle migrate unified: could not parse '{s}': {s}\n", .{ rel_path, `@errorName`(err) }); | |
| summary.parse_errors += 1; | |
| return; | |
| }; | |
| defer parsed.deinit(); | |
| const basename = basenameNoExt(entry_name); | |
| var counts = FileCounts{}; | |
| const ctx = TransformCtx{ | |
| .basename = basename, | |
| .xrefs = xrefs, | |
| .rel_path = rel_path, | |
| }; | |
| const out = try transformBytes(file_alloc, raw, parsed.value, ctx, &counts); |
🤖 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/walk.zig` around lines 210 - 238, migrateFile is still doing
the second-pass work in the shared long-lived arena, so per-file allocations for
raw, stripped, the parsed JSON tree, and transformBytes outputs accumulate
across the whole run. Move the second-pass allocations to a temporary per-file
arena that is reset/deinitialized after each file, while keeping only the needed
write-out result and counters. Make sure the per-file arena is used around
dir.readFileAlloc, stripJsoncToJson, std.json.parseFromSlice, and transformBytes
so each file’s memory is released before processing the next one.
1 participant
Pure mechanical split of the 3514-line
src/cli/migrate.ziginto cohesive submodules undersrc/cli/migrate/, each under the 1000-line limit.migrate.zigmigrate/scanner.zigmigrate/transforms.zigmigrate/transforms_meta.zigmigrate/pipeline.zigtransformBytesdispatcher + Summary/FileCounts/TransformCtxmigrate/walk.zigmigrate/tests*.zigPure move — behavior preserved by construction
Every function body diffs to zero non-trivial changes vs the original; the only deltas are mechanical ( on moved fns + module-qualifier prefixes on call sites). One-directional dependency DAG, no cycles (shared
Summary/FileCounts/TransformCtxlive inpipeline.zig, imported bywalk.zig).Verified
zig buildclean;zig build testgreen — 286 tests, unchanged from baseline (the migrate zspec specs were preserved via re-export so therunAlltest block still walks them — a comptime @import silently dropped 34, fixed). Smoke:migrate --help+ an end-to-end migration on a fixture fired all 9 transforms correctly.Note
Premise correction: migrate.zig wasn't test-free — it had ~1200 lines of zspec specs (a plain
^test "grep misses zspec's describe/it). So there WAS a safety net. Follow-up worth filing: the specs testtransformBytesdirectly but never exercisewalk.zig's file-I/O path — golden end-to-end fixtures would close that gap.Part of the >1000-line cleanup audit.