Repository navigation
packs: invisible <pack>__ namespacing + hooks scan + local→prefixed .jsonc rewrite (#440) - #480
Conversation
…ewrite Packs Phase 4 (RFC §4): pack-contributed component / prefab / pack-local event / hook names are registered in the generated global registry under the invisible `<pack>__<Name>` prefix (derived from the pack manifest name, mirroring `<plugin>__event`). Authors never type the prefix. - Prefix derivation `scan.packNamespacePrefix` with the save-stability note: the prefixed name is the save key (serde.componentName), so a pack `name` is save-stable (rename = migration). - Local→prefixed ref rewrite: `scanPack` rewrites a pack's own copied prefab JSONC so local component keys (`"Worker"`) become `"citizens__Worker"`, matching the emitted registry field. Only pack-owned component KEYS are rewritten; built-ins (`Position`), string values, and comments are left alone (`scan.rewritePackComponentKeys`). - hooks/ scanning (#439 deferred this): `scanPack` now scans `hooks/*.zig`; pack hooks are imported, added to the `GameHooks` receiver tuple, and instantiated under the `<pack>__` ident prefix. - Component registry field, event import alias + union variant, prefab registration key, and hook idents are all prefixed consistently. Tests: pack_scan_tests emission tests updated to the prefixed forms; new tests cover the prefab ref rewrite (end-to-end with the registry), pack hook emission, and the pure rewrite helper (keys-only, comment/value safety). Part of #651
📝 WalkthroughWalkthroughThis PR adds pack-scoped ChangesPack Namespacing Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![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
🤖 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/blocks/hooks.zig`:
- Around line 131-143: Pack names can collide after sanitization in the hooks
codegen path, causing duplicate `<pack>__...` symbols and broken generated
imports/tuples. Add a cross-pack uniqueness check during pack scanning, before
`writeHooksInitBlock`/the `self.pack_scans` loop in `hooks.zig`, using
`scan.packNamespacePrefix`-compatible sanitized prefixes so names like `my-pack`
and `my_pack` are rejected or surfaced as conflicts. Keep the check near the
pack collection/build step so `pack.name` collisions are caught before
`packNamespacePrefix`, `pathToIdent`, and `w.print` generate output.
🪄 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: 81a1a3a6-cf1e-4bdd-88c5-64b677f4f93b
📒 Files selected for processing (10)
src/codegen/blocks/events.zigsrc/codegen/blocks/hooks.zigsrc/codegen/blocks/imports.zigsrc/codegen/blocks/registries.zigsrc/codegen/context.zigsrc/codegen/lifecycle/callback.zigsrc/codegen/lifecycle/loop.zigsrc/codegen/scan.zigsrc/root.zigtest/pack_scan_tests.zig
![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: 040aa19abe
ℹ️ 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".
| for (pack.prefab_names) |name| { | ||
| const display = std.fs.path.basename(name); | ||
| try w.print(" try JsoncBridge.addEmbeddedPrefab(&g, \"{s}\", @embedFile(\"{s}/prefabs/{s}.jsonc\"), \"{s}/prefabs\");\n", .{ display, pack.import_prefix, name, pack.import_prefix }); | ||
| try w.print(" try JsoncBridge.addEmbeddedPrefab(&g, \"{s}__{s}\", @embedFile(\"{s}/prefabs/{s}.jsonc\"), \"{s}/prefabs\");\n", .{ prefix, display, pack.import_prefix, name, pack.import_prefix }); |
There was a problem hiding this comment.
When a pack prefab refers to another prefab from the same pack with a normal JSONC reference like { "prefab": "worker" }, this registration no longer creates a worker entry: only citizens__worker is added. The new rewritePackPrefabRefs pass only rewrites component keys, not prefab string values, so pack-local prefab composition will still look up the old bare name and fail at runtime; either rewrite pack-owned prefab references alongside component keys or keep a bare alias for local refs.
Useful? React with 👍 / 👎.
| const is_key = nextSignificantIsColon(src, j + 1); | ||
| if (is_key and containsKey(local_keys, content)) { |
There was a problem hiding this comment.
This rewrites any JSON object key whose text matches a pack-owned component, even when that key is part of component payload data rather than a component declaration. For example, if the pack owns Worker and a prefab contains { "components": { "Spawner": { "counts": { "Worker": 3 } } } }, the nested data key becomes citizens__Worker, changing the serialized payload before the component deserializer sees it. The rewrite needs to track JSONC context and only touch component-reference maps/flat entity component sites.
Useful? React with 👍 / 👎.
| const ident = eventVariantName(name); | ||
| const pascal = pathToPascal(name, pascal_buf); | ||
| try w.print(" {s}: {s}.{s},\n", .{ ident, ident, pascal }); | ||
| try w.print(" {s}__{s}: {s}__{s}.{s},\n", .{ prefix, ident, prefix, ident, pascal }); |
There was a problem hiding this comment.
Pack event tags are now emitted as citizens__worker_died, but pack hooks are imported as their original structs with no adapter or method-name rewrite. The existing hook contract requires handler function names to match GameEvents variant tags, so a pack that naturally defines pub fn worker_died(...) for its own events/worker_died.zig will not receive the event unless the author manually types the supposedly invisible prefix. Either keep pack-local hook dispatch names local or generate a wrapper that maps local handler names to the prefixed tags.
Useful? React with 👍 / 👎.
| const is_key = nextSignificantIsColon(src, j + 1); | ||
| if (is_key and containsKey(local_keys, content)) { |
There was a problem hiding this comment.
This rewrites any JSON object key whose text matches a pack-owned component, even when that key is part of component payload data rather than a component declaration. For example, if the pack owns Worker and a prefab contains { "components": { "Spawner": { "counts": { "Worker": 3 } } }, the nested data key becomes citizens__Worker, changing the serialized payload before the component deserializer sees it. The rewrite needs to track JSONC context and only touch component-reference maps/flat entity component sites.
Useful? React with 👍 / 👎.
| pub fn packNamespacePrefix(pack_name: []const u8, buf: *[128]u8) []const u8 { | ||
| return sanitizePluginIdent(pack_name, buf); |
There was a problem hiding this comment.
Different pack names can sanitize to the same namespace prefix here, for example my-pack and my_pack both become my_pack. If those packs both ship a local Worker component, worker prefab, or worker_died event, the generated supposedly namespaced identifiers/registry keys collide again (my_pack__Worker, my_pack__worker, etc.), defeating the cross-pack collision fix. Add a generate-time uniqueness check or use an injective escape for pack prefixes.
Useful? React with 👍 / 👎.
…ck names Sync past #479 (pack manifest early-gate + PackEntry/pack_entries reuse). Conflict in src/root.zig resolved keeping BOTH: #479's early load+dependency-validation gate and pack_entries reuse in the pack-scan loop, AND #480's namespacing (scanPack hooks/ scan, <pack>__ prefix derivation, .jsonc key rewrite). CodeRabbit (Major): add generate-time pack-name collision gate. Two packs whose names sanitize to the same <pack>__ prefix (e.g. my-pack / my_pack) would emit duplicate namespaced symbols and break the generated imports/registries/hook tuples. New pure pack_validate.checkPrefixCollisions (uses scan.packNamespacePrefix) runs alongside the depends_on gate in root.zig over pack_entries' plugin names; fails with error.PackNamePrefixCollision naming both packs. Tests added.
|
Synced to current main and addressed the CodeRabbit finding. Sync past #479. CodeRabbit (Major) — reject colliding pack names before codegen. Added a generate-time gate: two packs whose names sanitize to the same
Verification. Current status: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pack_validate.zig (1)
196-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
reportCyclebypasses the threaded allocator.
checkAcyclic/visitexplicitly thread anallocatorparam, butreportCyclehardcodesstd.heap.page_allocatorfor the diagnostic string instead of accepting the same allocator. It's error-path-only code so impact is minor, but it's an inconsistent allocator contract, and thecatch {}onappendSlicesilently swallows OOM, potentially producing a truncated cycle message.♻️ Thread the allocator through
-fn reportCycle(packs: []const PackDep, path: []const usize, back_to: usize) void { +fn reportCycle(allocator: std.mem.Allocator, packs: []const PackDep, path: []const usize, back_to: usize) void { ... - var buf: std.ArrayList(u8) = .empty; - defer buf.deinit(std.heap.page_allocator); + var buf: std.ArrayList(u8) = .empty; + defer buf.deinit(allocator); for (path[start..]) |node| { - buf.appendSlice(std.heap.page_allocator, packs[node].name) catch {}; - buf.appendSlice(std.heap.page_allocator, " -> ") catch {}; + buf.appendSlice(allocator, packs[node].name) catch {}; + buf.appendSlice(allocator, " -> ") catch {}; } - buf.appendSlice(std.heap.page_allocator, packs[back_to].name) catch {}; + buf.appendSlice(allocator, packs[back_to].name) catch {};(and pass
allocatorat the call site invisit)🤖 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/pack_validate.zig` around lines 196 - 221, `reportCycle` is hardcoding `std.heap.page_allocator` and swallowing allocation failures, which breaks the allocator flow used by `checkAcyclic` and `visit`. Update `reportCycle` to accept the same `allocator` parameter, pass it from the `visit` call site, and use it for the `std.ArrayList` appends/deinit so the diagnostic string is built consistently. Also avoid silent `catch {}` on `appendSlice`; handle allocation errors explicitly so cycle reporting does not truncate without notice.
🤖 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/pack_validate.zig`:
- Around line 196-221: `reportCycle` is hardcoding `std.heap.page_allocator` and
swallowing allocation failures, which breaks the allocator flow used by
`checkAcyclic` and `visit`. Update `reportCycle` to accept the same `allocator`
parameter, pass it from the `visit` call site, and use it for the
`std.ArrayList` appends/deinit so the diagnostic string is built consistently.
Also avoid silent `catch {}` on `appendSlice`; handle allocation errors
explicitly so cycle reporting does not truncate without notice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a3804867-b6aa-44b9-866b-8c7456eded1e
📒 Files selected for processing (2)
src/pack_validate.zigsrc/root.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/root.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0adf34572c
ℹ️ 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".
| const ident = eventVariantName(name); | ||
| const pascal = pathToPascal(name, pascal_buf); | ||
| try w.print(" {s}: {s}.{s},\n", .{ ident, ident, pascal }); | ||
| try w.print(" {s}__{s}: {s}__{s}.{s},\n", .{ prefix, ident, prefix, ident, pascal }); |
There was a problem hiding this comment.
In projects that declare a pack whose sanitized name matches an existing plugin/engine event prefix, this emits pack events into the same tag namespace as PluginEvents. For example, discoverPluginEvents reserves engine tags like engine__tick, so a pack named engine with events/tick.zig now also emits engine__tick; when GameEventsRaw is merged with PluginEvents, the duplicate union tag breaks the build. The new prefix-collision check only compares packs to other packs, so either reserve/check pack prefixes against plugin/engine event prefixes or use a pack-specific namespace.
Useful? React with 👍 / 👎.
| const prefix = scan.packNamespacePrefix(pack.name, &pack_prefix_buf); | ||
| for (pack.hook_names) |name| { | ||
| const ident = pathToIdent(name, ident_buf); | ||
| try w.print("const {s}__{s} = @import(\"{s}/hooks/{s}.zig\");\n", .{ prefix, ident, pack.import_prefix, name }); |
There was a problem hiding this comment.
When a pack contains both an event and a hook with the same plain stem, such as events/hit.zig and hooks/hit.zig, this hook import emits const citizens__hit = ..., and the pack event import later emits the same top-level alias for the event module. That makes an otherwise natural pack layout fail to compile as soon as hooks are scanned; use a disjoint alias namespace for hooks or reject these collisions during pack scan.
Useful? React with 👍 / 👎.
| var pack_names: std.ArrayList([]const u8) = .empty; | ||
| defer pack_names.deinit(allocator); | ||
| try pack_names.ensureTotalCapacity(allocator, pack_entries.items.len); | ||
| for (pack_entries.items) |e| pack_names.appendAssumeCapacity(e.plugin.name); |
There was a problem hiding this comment.
This validation only compares pack names with other pack names, so a game root can still define an already-prefixed name that collides with the generated pack key, e.g. prefabs/citizens__worker.jsonc with pack citizens/prefabs/worker.jsonc, or events/citizens__hit.zig with pack citizens/events/hit.zig. Generation then emits the same prefab registration key or event tag twice; include the scanned root event/prefab names in the reserved-prefix check before emitting.
Useful? React with 👍 / 👎.
Fix three correctness bugs found by chatgpt-codex in the <pack>__ namespacing feature (PR #480): 1. Pack-local prefab references were not rewritten. A pack prefab that composes another same-pack prefab via `{ "prefab": "worker" }` broke: only `citizens__worker` is registered while the reference stayed bare `worker`. rewritePackPrefabRefs now rewrites `"prefab"` value sites that name a pack-owned prefab (leaving foreign refs alone), and its guard is gated on prefab count — not component count — so a component-less pack with prefab-to-prefab refs is still rewritten. 2. Component-key rewrite corrupted payload data. The old rewrite matched ANY object key equal to a pack component name, including payload keys nested inside a component value (e.g. Spawner.counts.Worker). The rewriter is now context-aware: it tracks object nesting and only rewrites keys that are direct members of a `components`/`overrides` map. Documented boundary: the flat RFC #596 shape is intentionally not rewritten (conservative-but-correct, never corrupts payloads). 3. Pack hook handlers no longer matched prefixed event tags. Pack events fold into GameEvents as `<pack>__<event>`, but a pack hook's natural `pub fn worker_died` would (a) never receive the event and (b) hard @CompileError in the dispatcher's handler-name check. The copied hook source is now rewritten so a qualifying handler (pub, 2 params, name == a pack event) is renamed to the prefixed tag, keeping the prefix invisible while engine/plugin-event handlers keep their bare names. Consolidated the JSONC rewrite into a single context-aware rewritePackLocalRefs pass (rewritePackComponentKeys kept as a shim). Extends test/pack_scan_tests.zig + scan.zig unit tests for all three.
codex findings addressed (3/3) —
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements namespacing for pack-contributed components, events, prefabs, and hooks under an invisible <pack>__ prefix to prevent naming collisions between different packs and the game root. It introduces automated rewriting of local references in copied prefab JSONC files and hook handler names in Zig source files during the scan phase. Additionally, a validation gate is added to detect and reject packs with colliding sanitized namespace prefixes. As there are no review comments provided, I have no feedback to evaluate.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.zig`:
- Around line 428-435: The hook-name collection in the members scan is too broad
and is matching any public two-parameter function, including top-level helpers
and nested helper structs. Update the logic around the members loop and
ast.fullFnProto to scope renames to the actual hook receiver type by passing the
hook stem/Pascal receiver into the rewrite and only collecting names within that
container. Keep using event_names and fnProtoParamCount, but add a containment
check tied to the receiver type before treating a function as an event handler.
- Around line 222-224: The scan logic in the object handling path is incorrectly
treating nested payload “overrides” fields as component maps, so payload data
can be rewritten as component names. Update the object-state tracking in
scan.zig around the component-map detection in the code that uses pending_key,
and gate “overrides” so it only opens a component map when the current object is
a prefab/entity patch scope, not when the current value is already a component
payload. Apply the same fix in the related nested-object handling section
referenced by the second occurrence so that component payload values like
Spawner.overrides are preserved.
🪄 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: ba49ef4a-9d36-4925-9803-66e59741fff5
📒 Files selected for processing (3)
src/codegen/scan.zigsrc/root.zigtest/pack_scan_tests.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- test/pack_scan_tests.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ddda4f7d8
ℹ️ 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".
| }; | ||
| defer allocator.free(src); | ||
|
|
||
| const rewritten = try scan.rewritePackLocalRefs(allocator, src, keys.items, prefab_names, prefix); |
There was a problem hiding this comment.
For pack prefabs in subdirectories, scanner records names like enemies/goblin, but the lifecycle registration uses std.fs.path.basename(name) and registers only citizens__goblin. Passing the raw prefab_names here means a normal reference { "prefab": "goblin" } is left bare and no longer resolves after namespacing, while { "prefab": "enemies/goblin" } is rewritten to citizens__enemies/goblin, which is not the registered key either. This breaks same-pack composition for nested pack prefabs; rewrite against the same basename keys used by addEmbeddedPrefab or register the full names.
Useful? React with 👍 / 👎.
| // Value position: rewrite a pack-owned prefab reference | ||
| // (`"prefab": "worker"` → `"prefab": "citizens__worker"`). | ||
| if (pending_key) |k| { | ||
| if (std.mem.eql(u8, k, "prefab") and containsKey(prefab_names, content)) { |
There was a problem hiding this comment.
When a component payload contains its own field named prefab, for example { "components": { "Spawner": { "prefab": "worker" } } }, this condition rewrites the string to citizens__worker whenever the pack also owns a worker prefab. That field is component data, not an entity reference site, so the component deserializer now receives changed payload; track the JSONC entity/reference context before rewriting prefab values, similar to the component-key context guard.
Useful? React with 👍 / 👎.
| if (fp.visib_token != null) { | ||
| if (fp.name_token) |nt| { | ||
| const name = ast.tokenSlice(nt); | ||
| if (containsKey(event_names, name) and fnProtoParamCount(ast, fp) == 2) { |
There was a problem hiding this comment.
For pack events under subdirectories, event_names contains the path such as combat/worker_died, but the emitted event tag uses eventVariantName(name) and becomes citizens__worker_died. A natural pack hook pub fn worker_died(...) will not satisfy this raw containsKey(event_names, name) check, so it stays unprefixed and fails to receive the pack event; compare against the same basename variant names used during event emission.
Useful? React with 👍 / 👎.
| const ident = eventVariantName(name); | ||
| const pascal = pathToPascal(name, pascal_buf); | ||
| try w.print(" {s}: {s}.{s},\n", .{ ident, ident, pascal }); | ||
| try w.print(" {s}__{s}: {s}__{s}.{s},\n", .{ prefix, ident, prefix, ident, pascal }); |
There was a problem hiding this comment.
When pack names or event stems contain the __ separator, distinct pack-local events can still collapse to the same generated tag: pack a with events/b__hit.zig and pack a__b with events/hit.zig both emit a__b__hit here. The new prefix-collision gate allows those prefixes because a and a__b differ, so generation proceeds until duplicate event aliases/union tags break the build; escape one side injectively or validate the full generated event names after scanning.
Useful? React with 👍 / 👎.
Root cause 1 (JSONC rewrite context, CR L224 / codex L288):
rewritePackLocalRefs now tracks a precise object Scope (entity /
component_map / payload / array_*) instead of a bool component-map
flag. A `components`/`overrides` object only opens a component map when
its parent is an ENTITY scope, and a `prefab` value is only an entity
reference at ENTITY scope — so nested `overrides`/`prefab` inside a
component payload (`{ "Spawner": { "overrides": { "Worker": 3 } } }`,
`{ "Spawner": { "prefab": "worker" } }`) is no longer corrupted.
Root cause 2 (subdir pack items use basenames, codex L704 / L435):
prefab refs are matched + rewritten by BASENAME (`enemies/goblin` and
bare `goblin` both -> `<pack>__goblin`), matching addEmbeddedPrefab's
`std.fs.path.basename` key; the hook-rename event match compares
handler names against `eventVariantName(name)` (basename), so a subdir
event `combat/worker_died` matches `pub fn worker_died`.
Root cause 3 (hook rename too broad, CR L435):
rewritePackHookHandlerNames takes the hook stem and scopes the rename
to the DIRECT members of the receiver container (`pathToPascal(stem)`
struct) — top-level helpers / unrelated helper structs are never
renamed.
Root cause 4 (`<pack>__<event>` not injective, codex events L164):
new generate-time gate pack_validate.checkEmittedNameCollisions
validates the fully-qualified component/event/prefab names emitted
across the game root + every pack, rejecting duplicates (e.g. pack `a`
+`b__hit` vs pack `a__b`+`hit` both -> `a__b__hit`, and subdir prefab
basename clashes). Chose validation over delimiter-escaping to keep
save keys / emitted symbols stable — noted in the fn doc.
Tests: nested overrides/prefab-in-payload not rewritten; subdir prefab
ref + subdir event hook resolve to basename-prefixed names; top-level
helper pub fn not renamed; a/a__b full-name collision rejected.
Namespacing edge-case fixes (CodeRabbit + codex) — pushed in db8a3caConfirmed each finding against the code, fixed the root causes so they converge, and extended the test suite. Root cause 1 — JSONC rewrite context (CR L224 / codex L288)
Matches the engine's Root cause 2 — subdir pack items (codex L704 / L435)
Root cause 3 — hook rename too broad (CR L435)
Root cause 4 —
|
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.zig`:
- Around line 400-404: The basename-only check in prefabBasenameMatch is too
broad and can rewrite foreign prefab paths that merely share the same final
segment. Update prefabBasenameMatch in scan.zig so bare refs still use basename
matching, but any source value with a path must only match an exact prefab path
from prefab_names instead of comparing std.fs.path.basename values; use the
existing prefabBasenameMatch helper and its callers to distinguish pathless vs
pathful refs before rewriting.
🪄 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: dc9cde55-b15a-4588-9821-0e56fcecdcc0
📒 Files selected for processing (3)
src/codegen/scan.zigsrc/pack_validate.zigsrc/root.zig
| 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; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid rewriting foreign prefab paths that only share a basename.
"shared/goblin" will match a pack prefab like "enemies/goblin" because only basenames are compared, so a foreign ref can be rewritten to citizens__goblin. Keep basename matching for bare refs, but require exact path equality when the source value includes a path.
🐛 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 = std.mem.eql(u8, content, content_base);
for (prefab_names) |p| {
- if (std.mem.eql(u8, std.fs.path.basename(p), content_base)) return content_base;
+ const prefab_base = std.fs.path.basename(p);
+ if (std.mem.eql(u8, content, p) or
+ (content_is_bare and std.mem.eql(u8, prefab_base, content_base)))
+ {
+ return prefab_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.
| 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; | |
| } | |
| fn prefabBasenameMatch(prefab_names: []const []const u8, content: []const u8) ?[]const u8 { | |
| const content_base = std.fs.path.basename(content); | |
| const content_is_bare = std.mem.eql(u8, content, content_base); | |
| for (prefab_names) |p| { | |
| const prefab_base = std.fs.path.basename(p); | |
| if (std.mem.eql(u8, content, p) or | |
| (content_is_bare and std.mem.eql(u8, prefab_base, content_base))) | |
| { | |
| return prefab_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.zig` around lines 400 - 404, The basename-only check in
prefabBasenameMatch is too broad and can rewrite foreign prefab paths that
merely share the same final segment. Update prefabBasenameMatch in scan.zig so
bare refs still use basename matching, but any source value with a path must
only match an exact prefab path from prefab_names instead of comparing
std.fs.path.basename values; use the existing prefabBasenameMatch helper and its
callers to distinguish pathless vs pathful refs before rewriting.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db8a3ca640
ℹ️ 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 (topScope(scope_stack.items) == .component_map and | ||
| containsKey(component_keys, content)) |
There was a problem hiding this comment.
When a pack prefab uses the RFC #596 flat form, e.g. { "prefab": "base", "Worker": {...} } or a child with a Worker sibling key, the enclosing scope is .entity, not .component_map, so this gate leaves the pack-owned component name bare. The registry now emits only .citizens__Worker for that component, and this repo explicitly accepts PascalCase flat component keys in prefab files (src/scene_manifest_test.zig has flat prefab cases), so valid flat pack prefabs fail to resolve their own components after namespacing. The fresh evidence is the new .component_map-only condition, which excludes the flat entity component sites.
Useful? React with 👍 / 👎.
| 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; |
There was a problem hiding this comment.
When a pack owns prefabs/worker.jsonc, any entity reference whose path merely ends in worker (for example a game/dependency reference like "prefab": "shared/worker") is treated as pack-owned because this drops the directory before matching. That changes a non-pack path into "citizens__worker", contrary to the rewrite contract that foreign prefab references stay bare; path-spelled values should only rewrite on an exact pack prefab path, while basename matching should be reserved for bare local references.
Useful? React with 👍 / 👎.
| // Any other object field on an entity (`meta`, etc.) is opaque. | ||
| break :blk .payload; |
There was a problem hiding this comment.
When a pack prefab uses the still-supported unified wrapper shape, e.g. { "root": { "prefab": "worker", "overrides": { "Worker": {} } } }, this default classifies the root object as .payload, so neither the same-pack prefab value nor the wrapped component/override keys inside it are rewritten. Those local names then remain bare even though the pack registers only citizens__worker and .citizens__Worker, so root-wrapped pack prefabs fail after namespacing.
Useful? React with 👍 / 👎.
1 participant
Implements #440 (Packs Phase 4, RFC §4 + §10 Q3). Part of #651.
Builds on #439's pack dir-scan. Pack-contributed items are now registered in the generated global registry under an invisible
<pack>__<Name>prefix, so a pack and the game root (or another pack) that both define e.g.Workercan no longer collide — while authors keep writing bare local names.Prefix scheme
scan.packNamespacePrefix(pack_name)derives the ident (sanitized like plugin idents, mirroring the existing<plugin>__eventconvention). Every registry ident a pack contributes is prefixed<pfx>__:.Worker = @import("packs/citizens/components/Worker.zig").Worker.citizens__Worker = @import(…).Workerworker_died: worker_died.WorkerDiedcitizens__worker_died: citizens__worker_died.WorkerDiedaddEmbeddedPrefab(&g, "worker", …)addEmbeddedPrefab(&g, "citizens__worker", …)const citizens__overlay = @import(…),*citizens__overlay.Overlay,&citizens__overlay_instThe imported decl name stays bare — it's the component type's own
pub const Workerinside the pack file. Only the registry-facing name is prefixed.Local → prefixed ref rewrite
This is what keeps the prefix invisible. During
scanPack, after copying a pack'sprefabs/,rewritePackPrefabRefs→scan.rewritePackComponentKeysrewrites the copied JSONC so a pack author's local component key ("Worker": {…}) becomes"citizens__Worker", byte-matching the emitted registry field. Only JSON object keys that exactly equal one of the pack's own scanned components are rewritten — built-in/engine names (Position,Sprite), string values, and JSONC comment text are left untouched. Rewrite runs against the copied (destination) files, so the source pack tree is never mutated.hooks/ scanning
#439 deferred
hooks/"wanting the prefix first".scanPacknow scanshooks/*.zigand registers them into the sameGameHooksreceiver pipeline as the game root's hooks — import + receiver-type tuple + per-instance init — all under the<pack>__ident prefix, with consistent ordering (game hooks → pack hooks → flow handlers) across the type tuple and the.receiversliteral.Save-stable name
Documented near the prefix derivation (
scan.packNamespacePrefix): the prefixed component name is the on-disk save key (serde.componentName, engine-side), so a pack'snameis save-stable — renaming a shipped pack changes every component's save key and is therefore a save migration, not a cosmetic rename.Tests
test/pack_scan_tests.zig:scanPackrewrites a prefab's local component ref tocitizens__Workerwhile leavingPositionand value/"Worker"alone — and the same pack drives the registry to.citizens__Worker(end-to-end)GameHooks, and instantiated under the prefixSCAN_PACKnow assertshook_namesis populatedsrc/codegen/scan.zig: unit tests forpackNamespacePrefixandrewritePackComponentKeys(keys-only, comment-safe, value-safe, no-match dupe).zig build+zig build testpass.Deferred (clean seams, unchanged)
global ++ ownregistry partition /PackView(Parity: engine now accepts flat-form pack-namespaced component keys — assembler classifiers don't #652-remainder)exposes/depends_onDAG + isolation (RFC §6)Summary by CodeRabbit