Repository navigation
feat(check): lint bare pack-component names in game-root scenes (#490) - #494
Merged
Merged
Conversation
A game-root scene/prefab must reference a pack component by its namespaced registry key (`citizens__Worker`), not the bare local name (`Worker`). A bare name silently no-ops as an unknown component (RFC #596) — the entity loads with the component missing and no error, a debugging trap that cost real time in the pack-colony demo. Add a `labelle check` lint (`scene-bare-pack-component`) that walks the game root's `scenes/` and `prefabs/` and flags any bare PascalCase component reference that: - is not a name the game root itself owns (its `components/*.zig`), and - matches a known pack component's un-prefixed name, emitting an actionable "did you mean 'citizens__Worker'?" finding (listing every candidate when ambiguous). A correct namespaced key is silent; a genuinely-unknown name with no pack match keeps today's behavior (the engine's warn-once path is the backstop there). Implemented as a LINT rather than an auto-rewrite: rewriting the game root's authored scenes is riskier (mutates source, can't disambiguate two packs exporting the same un-prefixed name) — tracked as a follow-up. Lives in `check` (the RFC §6 enforcement net), the sanctioned home per the issue's option 2. - src/scene_name_lint.zig: scope-aware JSONC ref collector + pure lint + dir walk, with unit tests (flat-form, component-map, bundles, children, ambiguity, game-owned suppression). - src/check.zig: new `scene_bare_pack_component` Rule + slug. - src/check_cmd.zig: build the pack (bare→namespaced) + game-owned name sets from discovery, walk scenes/ + prefabs/; two e2e fixture tests. - src/root.zig: register the module for test discovery. Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new ChangesScene bare pack-component lint
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CheckCmd as check_cmd.runLint
participant SceneLint as scene_name_lint
participant Findings as findings list
CheckCmd->>SceneLint: build pack_components & game_owned names
CheckCmd->>SceneLint: scanScenesDir(scenes/, pack_components, game_owned)
SceneLint->>SceneLint: collectComponentRefs per .jsonc file
SceneLint->>SceneLint: lintSource(refs vs pack_components/game_owned)
SceneLint-->>Findings: append scene_bare_pack_component findings
CheckCmd->>SceneLint: scanScenesDir(prefabs/, pack_components, game_owned)
SceneLint-->>Findings: append additional findings
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b84e1c3a
ℹ️ 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 refs = try collectComponentRefs(arena, src); | ||
| for (refs) |ref| { | ||
| // A name the game root itself owns is a legitimate bare reference. | ||
| if (containsStr(game_owned, ref.name)) continue; |
There was a problem hiding this comment.
When a pack happens to define a component with the same bare name as an engine-provided component, legitimate root scene uses of that built-in are now reported as violations because only game_owned names are exempt here. For example, writeComponentRegistryBlock always registers .VideoComponent for scene/prefab JSONC, but if a pack also ships components/video_component.zig, { "VideoComponent": ... } in the game root will fail labelle check and suggest pack__VideoComponent even though the bare built-in key is valid. Add the engine/plugin-provided component names to the exempt set before matching pack candidates.
Useful? React with 👍 / 👎.
apotema
added a commit
that referenced
this pull request
Jul 1, 2026
…ins, pack-script order/staleness/context) (#500) * fix(packs): address codex findings from #494/#496 — scene-lint builtins exemption, pack-script order/staleness/context Four codex-review findings merged with the packs batch (#494, #496): 1. scene_name_lint.zig — exempt engine/gfx built-in component names (e.g. `VideoComponent`, always registered by `writeComponentRegistryBlock`) from the bare-pack-component lint. Previously a bare built-in was falsely flagged and mis-suggested to `pack__VideoComponent` when a pack shipped a same-named component. Adds `builtin_component_names` + exemption in `lintSource`. 2. root.zig — scan each light pack's `scripts/` at its `.plugins` declaration-order position (interleaved with plugins) so `ScriptScanner`'s `plugin_index` reflects `.plugins` order. The old two-phase scan (all plugins, then all packs) pushed a pack declared before a plugin behind it, breaking the per-state script ordering contract. 3. root.zig — when a pack ships no `scripts/` source, prune any stale dest a prior `generate` copied and register nothing. `copyAndScanAbs` no-ops on a missing source without touching the dest, so leftover copied scripts were otherwise scanned + compiled. Extracted `scanPackScriptsAt` (guards source existence) for both #2 and #3. 4. validate.zig / registries.zig — exclude pack/plugin `context` scripts (`plugin_name != null`) from the GameContext sentinel. A pack's `scripts/context.zig` no longer flips `hasContextEntry`, and `AllScripts` keeps importing it (instead of silently dropping it). Tests: builtin-exemption lint test; pack-before-plugin ordering test; stale-dest prune + present-script tests; pack-context emission test; hasContextEntry game-vs-pack tests. Also wired `codegen/validate.zig` into root.zig's test aggregator so its tests run under `zig build test`. `zig build` + `zig build test` green (1134 tests pass). Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog * fix(packs): propagate non-FileNotFound errors when probing pack scripts/ (codex P2 on #500) `scanPackScriptsAt`'s source-`scripts/` existence probe used a catch-all that treated ANY openDir failure — AccessDenied (permissions / broken mount), NotDir (`scripts` is a file), etc. — the same as a missing directory: it pruned the generated copy and silently dropped the pack's scripts, producing an incomplete build with no error. Mirror `copyAndScanAbs`'s source-root open (scanner.copyAndScanRecursive) EXACTLY: tolerate ONLY `error.FileNotFound` (prune stale dest + skip) and PROPAGATE every other error. Test: `scanPackScriptsAt propagates a non-FileNotFound probe error and does NOT prune (#500 codex)` — a pack whose `scripts` path is a file yields error.NotDir, which must propagate while the pre-existing generated copy stays intact. `zig build` + `zig build test` green (1135 tests pass). Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
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.
Summary
A game-root scene/prefab must reference a pack component by its namespaced registry key (
citizens__Worker), not the bare local name (Worker). A bare name silently no-ops as an unknown component (RFC #596) — the entity loads with the component missing and no error. This is a nasty debugging trap (it cost real time in the pack-colony demo, where workers/workstations silently didn't exist until the keys were namespaced).This adds a
labelle checklint that catches it.What it does
New rule
scene-bare-pack-componentwalks the game root'sscenes/andprefabs/and flags any bare PascalCase component reference that:components/*.zig), andemitting an actionable "Did you mean 'citizens__Worker'?" finding (listing every candidate when two packs export the same un-prefixed name).
Precision over recall (mirrors the existing
check.zigrules):citizens__Worker) → silentLint vs. rewrite
Implemented as a lint, not an auto-rewrite. Rewriting the game root's authored scenes is riskier — it mutates authored source and can't disambiguate when two packs export the same un-prefixed name. The pack-local rewrite (
scan.rewritePackLocalRefs) only namespaces a pack's own refs at copy time; extending that to the game root is left as a follow-up. The lint lives incheck— the RFC §6 enforcement net and the issue's sanctioned option 2.Placement:
check(not generate)Put in
labelle checkonly. It's the enforcement net the packs RFC §6 already positions for exactly these "the compile wall can't catch it" cases, is fully unit-tested and pure, and doesn't touch the (more invasive) generate pipeline. A generate-time warning could reuse the same purescene_name_lintAPI later — noted as a follow-up.Files
src/scene_name_lint.zig(new) — scope-aware JSONC component-ref collector + pure lint + directory walk, with unit tests.src/check.zig— newscene_bare_pack_componentRule+ slug.src/check_cmd.zig— builds the pack (bare→namespaced) + game-owned name sets from the existing pack discovery, walksscenes/+prefabs/; two e2e fixture tests.src/root.zig— register the module for test discovery.Testing
zig build+zig build test --summary allpass: 1120 passed, 4 skipped (46/46 steps).Closes #490.
https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
Summary by CodeRabbit