refactor: split codegen/scan.zig into focused sub-modules (behavior-preserving) - #539
Merged
Conversation
…reserving) Turn the ~3300-line codegen/scan.zig into a thin barrel that re-exports a new scan/ sub-package split along cohesive seams. Pure module extraction: no logic, signature, or output changes. Every scan.<Name> call site is unchanged (same public names + type identities re-exported from the barrel). Sub-modules: - scan/sanitize.zig identifier sanitization (sanitizePluginIdent, pathToIdent) - scan/pack_refs.zig pack-namespace JSONC rewriting (PackScan, rewritePackLocalRefs, ...) - scan/pack_hooks.zig pack hook-handler renaming (rewritePackHookHandlerNames) - scan/plugin_events.zig plugin/engine Events discovery (PluginEvent, discoverPluginEvents, ...) - scan/flow_decls.zig FlowNodes/PinStyles/Coercions discovery (PluginFlowNode, ...) - scan/promote.zig game-script -> named-module promotion (PromotedScript, ...) Tests moved verbatim alongside their functions; the barrel pulls every sub-module into analysis so `zig build test` runs them all. zig build + zig build test green; golden/codegen suite byte-identical. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughSix new Zig modules are added under src/codegen/scan/: flow_decls.zig (FlowNode/PinStyle/Coercion discovery), pack_hooks.zig (hook handler renaming), pack_refs.zig (JSONC entity/component reference rewriting), plugin_events.zig (event discovery), promote.zig (script-to-module promotion), and sanitize.zig (identifier sanitization helpers). ChangesCodegen scan module additions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Codegen
participant discoverPluginFlowDecls
participant PluginRoots as "Plugin src/root.zig"
participant ScriptEntries
participant PluginFlowDecls
Codegen->>discoverPluginFlowDecls: run discovery
discoverPluginFlowDecls->>PluginRoots: read + scan (is_script=false)
discoverPluginFlowDecls->>ScriptEntries: filter plugin_name==null
discoverPluginFlowDecls->>ScriptEntries: read + scan (is_script=true)
discoverPluginFlowDecls-->>PluginFlowDecls: owned FlowNodes/PinStyles/Coercions
PluginFlowDecls-->>Codegen: return discovered decls
sequenceDiagram
participant Codegen
participant rewritePackLocalRefs
participant wrapFlatEntityComponents
participant rewriteWrappedShapeRefs
participant OutputBuffer
Codegen->>rewritePackLocalRefs: rewrite(src, component_keys, prefab_names, prefix)
rewritePackLocalRefs->>wrapFlatEntityComponents: normalize flat components
wrapFlatEntityComponents-->>rewritePackLocalRefs: wrapped JSONC
rewritePackLocalRefs->>rewriteWrappedShapeRefs: rewrite keys/prefab values
rewriteWrappedShapeRefs->>OutputBuffer: emit rewritten bytes
OutputBuffer-->>Codegen: rewritten source
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 refactors the codebase by splitting codegen/scan.zig into several smaller, focused files under src/codegen/scan/, including modules for flow declarations, pack hooks, pack references, plugin events, script promotion, and sanitization helpers. The review feedback highlights critical memory management issues: in flow_decls.zig, OutOfMemory errors are incorrectly swallowed during both the plugin and game-script scanning passes instead of being propagated. Additionally, in promote.zig, a potential memory leak is identified in collectPromotedScripts if appending to the array list fails after duplicating rel_path without a corresponding errdefer cleanup.
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.
Comment on lines
+531
to
+540
| scanFlowDeclsInSource( | ||
| allocator, | ||
| src, | ||
| plugin.name, | ||
| sanitized, | ||
| false, // is_script | ||
| &flow_nodes, | ||
| &pin_styles, | ||
| &coercions, | ||
| ) catch continue; // tolerate per-plugin parse failures |
There was a problem hiding this comment.
The scanFlowDeclsInSource function can fail with error.OutOfMemory (e.g., during AST parsing, string duplication, or array list appending). Swallowing this error via catch continue violates the documented design goal that OutOfMemory should remain a hard failure rather than being silently masked as a parse failure.\n\nWe should explicitly check for error.OutOfMemory and propagate it, while continuing for other parse/read failures.
scanFlowDeclsInSource(\n allocator,\n src,\n plugin.name,\n sanitized,\n false, // is_script\n &flow_nodes,\n &pin_styles,\n &coercions,\n ) catch |err| switch (err) {\n error.OutOfMemory => return error.OutOfMemory,\n else => continue,\n };
Comment on lines
+567
to
+576
| scanFlowDeclsInSource( | ||
| allocator, | ||
| src, | ||
| entry.rel_path, | ||
| sanitized, | ||
| true, // is_script | ||
| &flow_nodes, | ||
| &pin_styles, | ||
| &coercions, | ||
| ) catch continue; |
There was a problem hiding this comment.
Similarly to the plugin pass, any error.OutOfMemory occurring inside scanFlowDeclsInSource during the game-script pass should be propagated rather than silently swallowed by catch continue.
scanFlowDeclsInSource(\n allocator,\n src,\n entry.rel_path,\n sanitized,\n true, // is_script\n &flow_nodes,\n &pin_styles,\n &coercions,\n ) catch |err| switch (err) {\n error.OutOfMemory => return error.OutOfMemory,\n else => continue,\n };
Comment on lines
+86
to
+89
| const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized); | ||
| errdefer allocator.free(module_name); | ||
| const rel_path = try allocator.dupe(u8, fn_.module_import_path); | ||
| try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path }); |
There was a problem hiding this comment.
If out.append fails with OutOfMemory, the successfully allocated rel_path string will be leaked. Since rel_path is not yet added to out.items, the function-scope errdefer on line 68 cannot clean it up.\n\nAdding an errdefer allocator.free(rel_path); right after the allocation of rel_path ensures it is properly cleaned up on failure before ownership is transferred to the list.
const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized);\n errdefer allocator.free(module_name);\n const rel_path = try allocator.dupe(u8, fn_.module_import_path);\n errdefer allocator.free(rel_path);\n try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path });
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codegen/scan/flow_decls.zig`:
- Around line 522-541: The error handling in scanFlowDeclsInSource is swallowing
allocation failures by using catch continue, which can silently skip entries and
leave the registry incomplete. Update the call sites in the plugin scan loop and
the game-script path to let error.OutOfMemory propagate while still tolerating
only non-memory parse failures, and keep the existing identifiers like
scanFlowDeclsInSource and the surrounding readFileAlloc flow so the fix applies
in both locations.
In `@src/codegen/scan/pack_refs.zig`:
- Around line 1242-1247: The prefabBasenameMatch helper is matching and
rewriting foreign prefab references by basename only, which can cause
path-qualified refs like other/goblin to be rewritten to a pack-owned goblin
from a different location. Update prefabBasenameMatch in pack_refs.zig so it
only treats a basename match as eligible when the original prefab reference is
already unqualified or otherwise explicitly scoped to this pack, and keep
foreign/game-root prefabs unchanged. Use the existing prefabBasenameMatch and
pack prefab rewrite logic in this scan flow to ensure only safe local refs are
rewritten.
In `@src/codegen/scan/promote.zig`:
- Around line 75-91: The script promotion loop in promoteScriptModuleName’s
caller leaks the duplicated rel_path when out.append fails. Add an errdefer for
the allocator.dupe(u8, fn_.module_import_path) result immediately after
allocation, matching the existing errdefer on module_name and the pattern used
in flow_decls.zig, so both allocated fields are freed if the append to out does
not succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92b12452-fe73-4d5b-9734-b415510b4861
📒 Files selected for processing (7)
src/codegen/scan.zigsrc/codegen/scan/flow_decls.zigsrc/codegen/scan/pack_hooks.zigsrc/codegen/scan/pack_refs.zigsrc/codegen/scan/plugin_events.zigsrc/codegen/scan/promote.zigsrc/codegen/scan/sanitize.zig
Comment on lines
+522
to
+541
| const src = std.Io.Dir.cwd().readFileAlloc(io, root_path, allocator, .limited(8 * 1024 * 1024)) catch |err| switch (err) { | ||
| error.OutOfMemory => return error.OutOfMemory, | ||
| else => continue, | ||
| }; | ||
| defer allocator.free(src); | ||
|
|
||
| var name_buf: [128]u8 = undefined; | ||
| const sanitized = sanitizePluginIdent(plugin.name, &name_buf); | ||
|
|
||
| scanFlowDeclsInSource( | ||
| allocator, | ||
| src, | ||
| plugin.name, | ||
| sanitized, | ||
| false, // is_script | ||
| &flow_nodes, | ||
| &pin_styles, | ||
| &coercions, | ||
| ) catch continue; // tolerate per-plugin parse failures | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant regions around the call sites and function definition.
FILE="src/codegen/scan/flow_decls.zig"
echo "== Call-site region 1 =="
sed -n '500,585p' "$FILE"
echo
echo "== Definition search =="
rg -n "fn scanFlowDeclsInSource|scanFlowDeclsInSource\\(" "$FILE"
echo
echo "== Surrounding definition =="
# Print a wider slice around the function definition if found.
line=$(rg -n "fn scanFlowDeclsInSource" "$FILE" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+220))
sed -n "${start},${end}p" "$FILE"
fiRepository: labelle-toolkit/labelle-assembler
Length of output: 15259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for explicit OOM handling and parse-error behavior in this file.
FILE="src/codegen/scan/flow_decls.zig"
echo "== Error-related patterns =="
rg -n "OutOfMemory|parse\\(|ast\\.errors|catch continue|return error\\.|error\\." "$FILE"Repository: labelle-toolkit/labelle-assembler
Length of output: 588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/codegen/scan/flow_decls.zig"
# Map the file structure and print only the function/call-site slices.
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Function definition slice =="
nl -ba "$FILE" | sed -n '430,620p'Repository: labelle-toolkit/labelle-assembler
Length of output: 259
Propagate error.OutOfMemory from scanFlowDeclsInSource instead of catch continue. This turns allocation failures into silent skips and can leave the generated registry incomplete. Apply the same fix at the game-script call site too.
🤖 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/flow_decls.zig` around lines 522 - 541, The error handling
in scanFlowDeclsInSource is swallowing allocation failures by using catch
continue, which can silently skip entries and leave the registry incomplete.
Update the call sites in the plugin scan loop and the game-script path to let
error.OutOfMemory propagate while still tolerating only non-memory parse
failures, and keep the existing identifiers like scanFlowDeclsInSource and the
surrounding readFileAlloc flow so the fix applies in both locations.
Comment on lines
+1242
to
+1247
| fn prefabBasenameMatch(prefab_names: []const []const u8, content: []const u8) ?[]const u8 { | ||
| const content_base = std.fs.path.basename(content); | ||
| for (prefab_names) |p| { | ||
| if (std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid rewriting path-qualified foreign prefab refs by basename alone.
Line 1245 rewrites any reference whose basename matches a pack prefab, so "prefab": "other/goblin" is rewritten if the pack owns "enemies/goblin". That violates the “foreign/game-root prefab stays bare” contract and can bind authors to the wrong prefab.
Proposed fix
fn prefabBasenameMatch(prefab_names: []const []const u8, content: []const u8) ?[]const u8 {
const content_base = std.fs.path.basename(content);
+ const content_is_bare = content_base.len == content.len;
for (prefab_names) |p| {
- if (std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base;
+ if (std.mem.eql(u8, p, content)) return std.fs.path.basename(p);
+ if (content_is_bare and std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base;
}
return 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.
Suggested change
| fn prefabBasenameMatch(prefab_names: []const []const u8, content: []const u8) ?[]const u8 { | |
| const content_base = std.fs.path.basename(content); | |
| for (prefab_names) |p| { | |
| if (std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base; | |
| } | |
| return null; | |
| fn prefabBasenameMatch(prefab_names: []const []const u8, content: []const u8) ?[]const u8 { | |
| const content_base = std.fs.path.basename(content); | |
| const content_is_bare = content_base.len == content.len; | |
| for (prefab_names) |p| { | |
| if (std.mem.eql(u8, p, content)) return std.fs.path.basename(p); | |
| if (content_is_bare and std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base; | |
| } | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codegen/scan/pack_refs.zig` around lines 1242 - 1247, The
prefabBasenameMatch helper is matching and rewriting foreign prefab references
by basename only, which can cause path-qualified refs like other/goblin to be
rewritten to a pack-owned goblin from a different location. Update
prefabBasenameMatch in pack_refs.zig so it only treats a basename match as
eligible when the original prefab reference is already unqualified or otherwise
explicitly scoped to this pack, and keep foreign/game-root prefabs unchanged.
Use the existing prefabBasenameMatch and pack prefab rewrite logic in this scan
flow to ensure only safe local refs are rewritten.
Comment on lines
+75
to
+91
| for (flow_nodes) |fn_| { | ||
| if (!fn_.is_script) continue; | ||
| // Dedupe by sanitized module — one named module per script file. | ||
| var seen = false; | ||
| for (out.items) |p| { | ||
| if (std.mem.eql(u8, p.module_name[("script__".len)..], fn_.module_sanitized)) { | ||
| seen = true; | ||
| break; | ||
| } | ||
| } | ||
| if (seen) continue; | ||
| const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized); | ||
| errdefer allocator.free(module_name); | ||
| const rel_path = try allocator.dupe(u8, fn_.module_import_path); | ||
| try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path }); | ||
| } | ||
| return out.toOwnedSlice(allocator); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
rel_path leaks if out.append fails.
module_name is guarded with errdefer allocator.free(module_name) right after allocation, but rel_path (line 88) has no equivalent guard before the fallible out.append call on line 89. If append fails to grow the backing array (OOM), module_name is freed via its errdefer, but rel_path was never appended and never freed — it leaks. This mirrors the errdefer discipline already applied consistently in the sibling flow_decls.zig (every duped field gets its own errdefer before the append).
🐛 Proposed fix
const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized);
errdefer allocator.free(module_name);
const rel_path = try allocator.dupe(u8, fn_.module_import_path);
+ errdefer allocator.free(rel_path);
try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path });📝 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.
Suggested change
| for (flow_nodes) |fn_| { | |
| if (!fn_.is_script) continue; | |
| // Dedupe by sanitized module — one named module per script file. | |
| var seen = false; | |
| for (out.items) |p| { | |
| if (std.mem.eql(u8, p.module_name[("script__".len)..], fn_.module_sanitized)) { | |
| seen = true; | |
| break; | |
| } | |
| } | |
| if (seen) continue; | |
| const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized); | |
| errdefer allocator.free(module_name); | |
| const rel_path = try allocator.dupe(u8, fn_.module_import_path); | |
| try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path }); | |
| } | |
| return out.toOwnedSlice(allocator); | |
| for (flow_nodes) |fn_| { | |
| if (!fn_.is_script) continue; | |
| // Dedupe by sanitized module — one named module per script file. | |
| var seen = false; | |
| for (out.items) |p| { | |
| if (std.mem.eql(u8, p.module_name[("script__".len)..], fn_.module_sanitized)) { | |
| seen = true; | |
| break; | |
| } | |
| } | |
| if (seen) continue; | |
| const module_name = try promotedScriptModuleName(allocator, fn_.module_sanitized); | |
| errdefer allocator.free(module_name); | |
| const rel_path = try allocator.dupe(u8, fn_.module_import_path); | |
| errdefer allocator.free(rel_path); | |
| try out.append(allocator, .{ .module_name = module_name, .rel_path = rel_path }); | |
| } | |
| return out.toOwnedSlice(allocator); |
🤖 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/promote.zig` around lines 75 - 91, The script promotion loop
in promoteScriptModuleName’s caller leaks the duplicated rel_path when
out.append fails. Add an errdefer for the allocator.dupe(u8,
fn_.module_import_path) result immediately after allocation, matching the
existing errdefer on module_name and the pattern used in flow_decls.zig, so both
allocated fields are freed if the append to out does not succeed.
This was referenced Jul 5, 2026
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…rving) (#442 follow-up) (#543) Turn the ~1600-line src/manifest.zig into a thin barrel that re-exports a new manifest/ sub-package split along cohesive seams. Pure module extraction: no logic, signature, or output changes. Every manifest.<Name> call site is unchanged (same public names re-exported from the barrel). Sub-modules: - manifest/parse.zig game/pack realm struct parsing (Field, StructDecl, parseStructDir, parseStructFile, extractSavePolicy) - manifest/json.zig JSON emission (ManifestData, PackRealm, SCHEMA_VERSION, writeManifestJson + all writers) - manifest/emit.zig orchestration (MANIFEST_FILENAME, PackInput, emitManifestSidecar, writeSidecar) Tests moved verbatim alongside their functions; the barrel pulls every sub-module into analysis so `zig build test` runs them all. The only non-mechanical deltas vs the original are added `pub` qualifiers for cross-module access, `../`-rewritten imports, and module-qualified calls — a normalized set-diff confirms every output-affecting line is verbatim. Mirrors the codegen/scan.zig split in #539. zig build + zig build test green; zig fmt --check clean; manifest JSON writer byte-identical (verified by diff of the moved writer region). Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…(behavior-preserving) (#544) Pure extraction of the 1501-line `src/codegen/manifest_v2_splice.zig` into a `manifest_v2_splice/` sub-package along the per-platform seam, mirroring #539. The barrel re-exports the unchanged public surface; every symbol keeps its name and identity so `manifest_v2_splice.<Name>` call sites are untouched. - common.zig — cross-platform helpers (dep-option values, merge, module/artifact idents, generic walk splice, byte-anchor discriminator, hook staging, packaging, root build-deps) - desktop.zig — desktop byte-anchor + generic emitters + sokol residual - android.zig / ios.zig / wasm.zig — per-platform header/deps/backend-dep/link - dispatch.zig — the backend-dep + link section routers - manifest_v2_splice.zig — thin barrel (re-exports + test pull-in) Sub-file `@import`/`@embedFile` paths shifted one level deeper. Tests moved with the code they cover. No behavior change: `zig build` + `zig build test` green, golden byte-comparison suite zero diff, `zig fmt --check` clean. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…r-preserving) (#541) Pure extraction of the 1357-line src/plugin_manifest.zig into a src/plugin_manifest/ sub-package, mirroring PR #539. No behavior changes: byte-identical generated output (golden suite zero diff), public surface re-exported unchanged. Sections split along cohesive seams: - plugin_manifest/common.zig — shared version gate + reserved/safe name checks (SUPPORTED_MANIFEST_VERSION, RESERVED_DIR_NAMES, isReservedDirName, isSafeDirName) - plugin_manifest/plugin.zig — plugin.labelle schema + loaders (ConventionDir, PluginManifest, loadOptional, loadFromDir) - plugin_manifest/pack.zig — pack.labelle schema + loaders (PackManifest, PackExposes, loadPackOptional, loadPackFromDir, packDirHasDeclModuleContent) plugin_manifest.zig is now a thin barrel re-exporting every original symbol by its original name; sub-files are path-@import'd (no build.zig change). All tests moved with the code they exercise and are pulled back in via the barrel's `test {}` aggregator. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
This was referenced Jul 5, 2026
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…eserving) (#549) Pure extraction, mirrors #539/#541. `build_files.zig` was a single ~1165-line module; it is now a 35-line barrel re-exporting the public surface from two cohesive sub-modules under `build_files/`: - build_files/build_zig.zig — build.zig generation (sanitizeExeName, BuildZigOptions, generateBuildZig, emit* helpers, desktopUsesGenericV2, ...) - build_files/build_zig_zon.zig — build.zig.zon generation (BuildZigZonOptions, generateBuildZigZon, deps-link/fallback path, deps_linker re-export, v2BackendDepName, relativePath) Split along the natural build.zig-vs-build.zig.zon seam; no private helper crosses the boundary. Public surface (root.zig call sites) unchanged; every symbol keeps its name and identity. Sub-files are path-@import'd (no build.zig change). `zig build` + `zig build test` green; golden suite zero diff; `zig fmt --check` clean. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
Rebuilt onto the six parallel split-refactors (#539-#549): identical semantics, new homes — scanPack verb-copy in root/pack_scan.zig, surface+depends_on emission in build_files/build_zig.zig, the .all diagnostic in plugin_manifest/pack.zig. pack_root/pack_validate/ pack_refs/tests/docs carried verbatim (untouched by the splits). Includes the review fixes from the first head: @"…" escaping on exposed verb idents both sides, token-sequence .exposes = .all detection (exposesAllShorthand), docs aligned with header-only + targeted-diagnostic behavior. Claude-Session: https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…548) Rebuilt onto the six parallel split-refactors (#539-#549): identical semantics, new homes — scanPack verb-copy in root/pack_scan.zig, surface+depends_on emission in build_files/build_zig.zig, the .all diagnostic in plugin_manifest/pack.zig. pack_root/pack_validate/ pack_refs/tests/docs carried verbatim (untouched by the splits). Includes the review fixes from the first head: @"…" escaping on exposed verb idents both sides, token-sequence .exposes = .all detection (exposesAllShorthand), docs aligned with header-only + targeted-diagnostic behavior. Claude-Session: https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
This was referenced Jul 5, 2026
Merged
apotema
added a commit
that referenced
this pull request
Jul 5, 2026
…preserving) (#557) Splits the 1951-line cohesive-giant `src/codegen/scan/pack_refs.zig` into an acyclic module set, left intact during the scan split (#539): - `pack_refs/common.zig` — shared JSONC scanning primitives (skipTrivia/scanString/scanValue/scanBalanced), key classifiers (isPascalCase/isEntityListKey/containsKey), and the engine-parity container-shape probes (isOnlyMetaHeaderObject/bundleHeaderOpen/ rootWrapperValueOpen/bundleHeaderLegacyEntitiesOffset). - `pack_refs/pass1.zig` — flat->wrapped normalization (wrapFlatEntityComponents + the FlatWrap walker). - `pack_refs/pass2.zig` — the scope-tracked rewrite (rewriteWrappedShapeRefs + Scope model). - `pack_refs.zig` — thin barrel: PackScan, packNamespacePrefix, rewritePackComponentKeys, the rewritePackLocalRefs orchestrator, the re-exported bundleHeaderLegacyEntitiesOffset, and all unit tests. Acyclic by construction: pass1 -> common, pass2 -> common, barrel -> {pass1,pass2,common}; the two passes never import each other. The container-shape probes previously reached the scanners by constructing a throwaway FlatWrap; moving BOTH the scanners and the probes into common breaks that apparent cycle. FlatWrap keeps thin delegating scanner methods so its emit/parse bodies stay byte-for-byte unchanged. Behavior-preserving: no logic/signature changes; the scan.zig barrel's re-exported public surface is untouched. `zig build test` green, ZERO golden diff (test/goldens/ unchanged), `zig fmt --check` clean. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
src/codegen/scan.zig(3307 lines) was a cohesive-but-huge discovery module. This is a pure, behavior-preserving module extraction — the proof-of-pattern for the broader "split all >1000-line files" effort. No logic, signature, or generated-output changes.scan.zigis now an 87-line barrel that re-exports the public surface from a newsrc/codegen/scan/sub-package. Everyscan.<Name>call site across the repo is unchanged (same public names, same type identities re-exported from the barrel — no churn at any of the ~20 importers).Sections extracted (name — responsibility — original line range)
scan/sanitize.zigsanitizePluginIdent,pathToIdent(+stderrPrint)scan/pack_refs.zigPackScan,packNamespacePrefix,rewritePackComponentKeys,rewritePackLocalRefs,bundleHeaderLegacyEntitiesOffset, the two-pass walker (FlatWrap+rewriteWrappedShapeRefs) and helpersscan/pack_hooks.zigrewritePackHookHandlerNames(+ AST helpers)scan/plugin_events.zigEventsdiscovery:PluginEvent(s),discoverPluginEvents,discoverEventsFromRootscan/flow_decls.zigPluginFlowNode/PinStyle/Coercion/FlowDecls,discoverPluginFlowDecls,dedupePinStyles,scanFlowDeclsInSource,flowNodeIsVoid,extractConstructsStringscan/promote.zigPromotedScript,promotedScriptModuleName,collectPromotedScripts,freePromotedScriptsscan.zig(barrel)Tests were moved verbatim next to their functions. The barrel's
test { _ = <submodule>; ... }block pulls every sub-module into analysis sozig build testkeeps running the exact same specs.Dependency graph (no cycles)
sanitizeis a leaf.pack_refs → sanitize;plugin_events → sanitize;flow_decls → sanitize;promote → flow_decls(for thePluginFlowNodetype);pack_hooks → ../idents.zig. Shared file-private helpers (containsKey,isPascalCase) are used only withinpack_refsand stayed there. No newbuild.zigmodule registration — the sub-files are path-@imported within the same module.One file still >1000 lines (deliberate)
scan/pack_refs.zigis 1951 lines, of which ~1265 is code (the rest are the moved specs). This is a single cohesive concern — the pack-JSONC rewrite. Its pass-1 flat-wrap walker (FlatWrap) and pass-2 scope walk (rewriteWrappedShapeRefs) are mutually entangled: pass-2's shape probes (bundleHeaderOpen/rootWrapperValueOpen) reuseFlatWrap's scanning primitives, andrewritePackLocalRefsorchestrates both. Forcing a further split would create awkward cross-module coupling / circular imports for no cohesion gain, so it was left as one seam.Verification
zig buildcompiles on Zig 0.16.0.zig build testgreen (exit 0). Thefailed command … --listen=-anderror:/ParseZonstderr lines are the negative-path specs writing expected diagnostics (documented in the module doc-comment), not failures.@embedFiled and compared withexpectEqualStringsinsidetest/main_zig_tests.zigandtest/build_zig_tests.zig(generatedmain.zig+build.zig, both driven throughscan's discovery). A green suite means the generated output is byte-identical to the committed goldens.zig fmt --checkclean on the barrel and all six sub-modules.Branched off origin/main @ 85cd72e (#534).
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit
New Features
Bug Fixes