feat(packs): exposes surface modules + depends_on wiring (#498 PR 4) - #548
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR implements a manifest-driven "verb surface" contract for packs: scanning root-level queries.zig/commands.zig, generating a ChangesPack surface exposes/depends_on implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Generate as root.zig generate()
participant PackScan as scanPack
participant Validate as pack_validate.checkExposesFiles
participant Codegen as renderSurface
participant BuildZig as emitPackModules
Generate->>PackScan: scan pack directory
PackScan-->>Generate: has_queries, has_commands
Generate->>Validate: check exposes vs shipped files
Validate-->>Generate: ok or error.PackExposesMissingFile
Generate->>Codegen: renderSurface(pack_name, exposes)
Codegen-->>Generate: __surface.zig source
Generate->>BuildZig: pack module entry with depends_on
BuildZig->>BuildZig: emit pack_surface__<dep>_mod
BuildZig->>BuildZig: overrideImport depender -> surface module
Possibly related issues
Possibly related PRs
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 implements the exposes and depends_on verb surfaces for the Packs system, narrowing down a pack's public API to explicitly declared queries and commands. It introduces the generation of __surface.zig modules, wires dependencies in the build configuration, and adds validation to prevent exposing missing files or using the unsupported .exposes = .all shorthand. Feedback focuses on escaping generated Zig identifiers in __surface.zig to handle reserved keywords or special characters safely, and refining the substring search in plugin_manifest.zig to avoid false-positive diagnostics.
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.
| for (exposes.queries) |name| { | ||
| try w.print(" pub const {s} = pack.queries.{s};\n", .{ name, name }); | ||
| } |
There was a problem hiding this comment.
The exposed query names are directly interpolated as Zig identifiers in the generated __surface.zig file. If a manifest defines an exposed name that is a reserved Zig keyword (like const or fn) or contains special characters, the generated code will fail to compile. Using Zig's @"" identifier escaping syntax ensures that any valid string can be safely re-exported without causing syntax errors.
for (exposes.queries) |name| {
try w.print(" pub const @\"{s}\" = pack.queries.@\"{s}\";\n", .{ name, name });
}
There was a problem hiding this comment.
Applied in 47e5c68 — see commit message for the specifics (escaped idents both sides / token-sequence scan / docs aligned with the header-only + targeted-diagnostic behavior).
| for (exposes.commands) |name| { | ||
| try w.print(" pub const {s} = pack.commands.{s};\n", .{ name, name }); | ||
| } |
There was a problem hiding this comment.
The exposed command names are directly interpolated as Zig identifiers in the generated __surface.zig file. If a manifest defines an exposed name that is a reserved Zig keyword (like const or fn) or contains special characters, the generated code will fail to compile. Using Zig's @"" identifier escaping syntax ensures that any valid string can be safely re-exported without causing syntax errors.
for (exposes.commands) |name| {
try w.print(" pub const @\"{s}\" = pack.commands.@\"{s}\";\n", .{ name, name });
}
There was a problem hiding this comment.
Applied in 47e5c68 — see commit message for the specifics (escaped idents both sides / token-sequence scan / docs aligned with the header-only + targeted-diagnostic behavior).
| if (std.mem.indexOf(u8, raw_bytes, ".exposes") != null and | ||
| std.mem.indexOf(u8, raw_bytes, ".all") != null) |
There was a problem hiding this comment.
Performing a simple substring search for ".exposes" and ".all" on the entire raw_bytes can lead to false positives if either string appears in comments or unrelated fields. If the ZON parser fails for an entirely different reason, this warning might still be printed, potentially confusing the developer. Consider a more robust check (such as verifying the relative order or proximity of the tokens, or checking if they are not part of comments) to ensure the diagnostic is only shown when .exposes = .all is actually present in the active configuration.
There was a problem hiding this comment.
Applied in 47e5c68 — see commit message for the specifics (escaped idents both sides / token-sequence scan / docs aligned with the header-only + targeted-diagnostic behavior).
![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
🧹 Nitpick comments (1)
src/codegen/pack_root.zig (1)
199-258: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider validating exposed names are valid Zig identifiers.
renderSurfaceemitspub const {name} = pack.queries.{name};directly from manifest-supplied strings. An exposes entry containing invalid identifier characters (hyphens, spaces, leading digit) produces a syntax error in the generated__surface.zigrather than a clear manifest-validation message pointing at the offending entry inpack.labelle. Compilation still fails safely, so this is a diagnostics-quality gap rather than a functional bug.♻️ Possible approach
Add an identifier-format check alongside
checkExposesFiles(inpack_validate.zig) that rejects anexposesentry which isn't a valid Zig identifier, with an error naming the pack and the bad entry.🤖 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/pack_root.zig` around lines 199 - 258, `renderSurface` currently emits manifest-supplied expose names directly into `__surface.zig`, so invalid Zig identifiers only fail later as generated-code syntax errors. Add validation for `exposes` entries in the pack validation path (near `checkExposesFiles` in `pack_validate.zig`) to reject any query/command name that is not a valid Zig identifier, and report the pack plus the offending entry in the error message. Keep `renderSurface`, `SurfaceExposes`, and the generated `pub const {name}` exports unchanged except for relying on the new validation.
🤖 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 `@docs/packs.md`:
- Around line 81-89: Update the surface-semantics wording in the docs to match
the actual `__surface.zig` behavior: in the pack import description, distinguish
the populated-surface case from the empty/null `exposes` case, since empty/null
`exposes` should produce a header-only `__surface.zig` rather than importing the
pack module. Also revise the `.exposes = .all` wording in the `@import("<dep>")`
/ `contracts` section to say it triggers a targeted validation diagnostic from
the manifest/validation path, not a generic parse error, while keeping the
references to `@import("<dep>")`, `__surface.zig`, and `contracts` aligned with
the implementation.
---
Nitpick comments:
In `@src/codegen/pack_root.zig`:
- Around line 199-258: `renderSurface` currently emits manifest-supplied expose
names directly into `__surface.zig`, so invalid Zig identifiers only fail later
as generated-code syntax errors. Add validation for `exposes` entries in the
pack validation path (near `checkExposesFiles` in `pack_validate.zig`) to reject
any query/command name that is not a valid Zig identifier, and report the pack
plus the offending entry in the error message. Keep `renderSurface`,
`SurfaceExposes`, and the generated `pub const {name}` exports unchanged except
for relying on the new validation.
🪄 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: 804d4f7f-c5b2-4925-876b-0ee64072bd95
📒 Files selected for processing (8)
docs/packs.mdsrc/build_files.zigsrc/codegen/pack_root.zigsrc/codegen/scan/pack_refs.zigsrc/pack_validate.zigsrc/plugin_manifest.zigsrc/root.zigtest/pack_scan_tests.zig
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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/root.zig (1)
848-855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the repeated "find this pack's manifest entry" lookup.
The same
pack_entrieslinear-search-by-name pattern is duplicated here twice (once resolving viaunreachableon a miss, once silently defaulting to&.{}on a miss). Both are currently safe by construction (pack_scans/pack_modulesare always built frompack_entriesin lockstep), but the inconsistent miss-handling means a future refactor that breaks that invariant would silently wire emptydepends_onhere instead of loudly failing like the other site. Consider zip-iteratingpack_scans/pack_modulestogether withpack_entries(as already done at line 1144) or extracting a single helper that always usesunreachableon a miss, to avoid the O(n²) duplicate lookups and the inconsistent fallback behavior.Also applies to: 991-996
🤖 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/root.zig` around lines 848 - 855, Consolidate the repeated pack manifest lookup in root.zig so both call sites use the same lookup path and failure behavior. The current `pack_entries` linear search is duplicated in the `exposes` block and the related `depends_on` handling, with one branch using `unreachable` and the other falling back silently; refactor this by either zip-iterating `pack_scans`/`pack_modules` with `pack_entries` as done elsewhere, or extracting a helper that resolves the manifest entry for a pack name and միշտ fails loudly on a miss. Keep the `pack_validate.checkExposesFiles` and `pack.name` flow unchanged, but eliminate the O(n²) duplicate search and the inconsistent empty-default behavior.
🤖 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/root.zig`:
- Around line 848-855: Consolidate the repeated pack manifest lookup in root.zig
so both call sites use the same lookup path and failure behavior. The current
`pack_entries` linear search is duplicated in the `exposes` block and the
related `depends_on` handling, with one branch using `unreachable` and the other
falling back silently; refactor this by either zip-iterating
`pack_scans`/`pack_modules` with `pack_entries` as done elsewhere, or extracting
a helper that resolves the manifest entry for a pack name and միշտ fails loudly
on a miss. Keep the `pack_validate.checkExposesFiles` and `pack.name` flow
unchanged, but eliminate the O(n²) duplicate search and the inconsistent
empty-default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76a80bf1-cb54-4f11-a3fb-7964da6998ee
📒 Files selected for processing (9)
docs/packs.mdsrc/build_files/build_zig.zigsrc/codegen/pack_root.zigsrc/codegen/scan/pack_refs.zigsrc/pack_validate.zigsrc/plugin_manifest/pack.zigsrc/root.zigsrc/root/pack_scan.zigtest/pack_scan_tests.zig
✅ Files skipped from review due to trivial changes (1)
- docs/packs.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pack_validate.zig
- src/codegen/pack_root.zig
- src/codegen/scan/pack_refs.zig
- test/pack_scan_tests.zig
1 participant
PR 4 of the #498 train — the
exposes/depends_onhalf of the wall.What lands
scanPackcopies a pack's root-levelqueries.zig/commands.zig(single-file variant of the feat(packs): scan a light pack's scripts/ into the per-state dispatch (#487) #496 stale-prune discipline; a pack may ship only a verb surface — the dest-dir edge the new test caught).__pack_root.zigre-exports them raw for the pack's own code.__surface.zig(newrenderSurface): generated per pack, re-exporting exactly the manifest'sexposeslists through the pack's"pack"self-import. A listed-but-missing verb fails compilation pointing at the generated surface; anull/emptyexposesyields a header-only module — dependents can call nothing, the correct default.depends_on. (Uniform emission was the first cut; the smoke project caught Zig'sunused local constanthard error in the generated build.zig for undepended surfaces.)depends_onentries naming sibling packs map the dep's plain name onto the dep's surface (overrideImport(pack__X_mod, "<dep>", pack_surface__<dep>_mod)) — dependents never seepack__<prefix>.contractsstays the implicit full-module import; plugin deps are already in every table.exposesnaming verbs from a file the pack doesn't ship →error.PackExposesMissingFilewith the manifest named (pack_validate.checkExposesFiles, unit-tested);.exposes = .all→ targeted diagnostic naming the explicit-list fix (the RFC shorthand is deliberately unsupported — an unbounded surface defeats the wall).Proof on the real two-pack project
production(depends_on = .{"citizens"}) calling@import("citizens").queries.find_idle(game)— builds green, runs through the surface → pack-module chain.error: struct '__surface.queries' has no member named 'internal_reset'depends_onremoved:error: no module named 'citizens' available within module 'pack0'(thepack0is Zig's display-dedup of the shared"pack"self-import name — cosmetic)Tests
Suite 46/46 steps, 1273/1277 (4 skipped, 0 failed) — new
PACK_SURFACEstruct: renderer shapes (exact re-export + header-only empty),checkExposesFilespositive/negative,scanPackverb-file copy + stale-prune round-trip, and the build-wiring asserts (surfacecreateModulewith sole-packimport, demand-driven declaration, dependent→surface direction with both negative directions pinned).docs/packs.mdgains the exposes/depends_on section with the authoring example and the error surfaces.Next: PR 5 (lint demotion + stale-comment truth-up +
root.zig/build_files.zig>1000-line splits), PR 6 (examples/packs-demo + CI e2e fixture).Part of #498
https://claude.ai/code/session_01P7YLw4hXFCCaY2LAUt4G1j
Summary by CodeRabbit
New Features
queriesandcommandsthrough generated surface modules.Bug Fixes
Documentation