Repository navigation
packs: check subcommand — the §6 enforcement lint (part of #270) - #486
Merged
Merged
Conversation
Add `labelle-assembler check --project-root <path>` — a static/token scan
over a game's packs that reports the Packs RFC §6 convention violations the
compile wall can't (yet) catch (the "net" layer of the enforcement stack).
Rules implemented:
1. cross-pack registry access on foreign names — getType/entityHasNamed
with a foreign `<other>__X` string, and view(.{ForeignComponent}) on an
unambiguously foreign component (the string-escape hole, RFC §6-1b).
2. raw `.global`-facet writes — direct `Locked{…}` construction in a
non-owner pack, bypassing the sanctioned API (`Locked.tryAcquire`).
3. event-direction inversions — a lower pack referencing a higher pack's
qualified event (`<higher>__<event>`), using the depends_on DAG that
pack_validate.zig already parses (#441). Subscription detection is
heuristic (qualified-name reference); the compile-time wall is future.
`src/check.zig` is the pure, std-only token scanner (14 unit tests);
`src/check_cmd.zig` is the driver: discovers packs via `pack.labelle`,
builds the owner maps + reverse-transitive DAG, walks each pack's `.zig`
files, prints `file:line:col [rule] message`, exits 1 on any finding.
Fixture-based end-to-end tests cover a violating tree (all 3 rules) and a
clean tree. Protocol bumped to v4.
Part of the Packs initiative (umbrella labelle-engine#651; RFC
moca-tecnologia/flying-platform-labelle#561 §6).
📝 WalkthroughWalkthroughAdds a new ChangesPacks Enforcement Lint
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/check_cmd.zig (1)
343-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
project.labellereader.The docstring itself flags this as a copy of main.zig's private helper. Two independent parsers for the same config format is a maintenance hazard — a future change to
project.labelleparsing (e.g. new fields, error handling) risks drifting between the two copies.Consider exporting the helper from
main.zig(or hoisting it intoconfig.zig) and having both call sites share it.🤖 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/check_cmd.zig` around lines 343 - 351, The `readProjectConfig` helper in `check_cmd.zig` is a duplicated parser for `project.labelle`, so replace this inline copy with a shared implementation. Move or export the config-reading logic from `main.zig` (or factor it into `config.zig`) and update `readProjectConfig` and the existing `main.zig` call site to use the same function so parsing, validation, and error handling stay in sync.
🤖 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/check_cmd.zig`:
- Around line 113-124: The resolve failure in discoverPacks is being swallowed
by catch continue, which can hide skipped packs from the lint scan. Update the
cache.resolvePlugin path in discoverPacks to log a warning with std.log.warn
before continuing, matching the manifest.loadPackFromDir error handling and
including the plugin.name and error name so runLint/report can accurately
reflect partial scans.
In `@src/check.zig`:
- Around line 356-377: The raw global facet write check in scanSource is
overmatching function return types, since an identifier followed by “{” is being
treated as a facet construction even in signatures like fn currentLock() Locked
{ ... }. Update the logic in the scanSource path around the current facet-name
matching and period guard to recognize function declarations/return-type
contexts and skip those cases before calling emit for .raw_global_facet_write.
Add a regression test covering fn ...() Locked {} so the checker only flags
actual facet writes, not return types.
---
Nitpick comments:
In `@src/check_cmd.zig`:
- Around line 343-351: The `readProjectConfig` helper in `check_cmd.zig` is a
duplicated parser for `project.labelle`, so replace this inline copy with a
shared implementation. Move or export the config-reading logic from `main.zig`
(or factor it into `config.zig`) and update `readProjectConfig` and the existing
`main.zig` call site to use the same function so parsing, validation, and error
handling stay in sync.
🪄 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: 7749083d-5494-496e-b22f-15ab02c82aff
📒 Files selected for processing (4)
src/check.zigsrc/check_cmd.zigsrc/main.zigsrc/root.zig
Comment on lines
+113
to
+124
| fn discoverPacks(arena: std.mem.Allocator, io: std.Io, cfg: ProjectConfig, root: []const u8) ![]const Pack { | ||
| var list: std.ArrayList(Pack) = .empty; | ||
| for (cfg.plugins) |plugin| { | ||
| const pack_dir = cache.resolvePlugin(arena, plugin, root) catch continue; | ||
| var manifest = (plugin_manifest.loadPackFromDir(arena, pack_dir, plugin.name) catch |err| { | ||
| // A malformed pack.labelle is a real problem, but the DAG gate | ||
| // (`pack_validate`) is where that's reported at generate time; | ||
| // the lint should not hard-fail on it. Skip and continue. | ||
| std.log.warn("labelle-assembler check: skipping pack '{s}': {s}", .{ plugin.name, @errorName(err) }); | ||
| continue; | ||
| }) orelse continue; // not a pack (no pack.labelle) | ||
| defer manifest.deinit(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Resolve failures are silently dropped without logging — undermines lint reliability.
cache.resolvePlugin failures are swallowed via catch continue (line 116) with no log message, unlike the manifest-load failure a few lines below (117-123) which does std.log.warn. Since pack_count (line 108/152) only reflects successfully-resolved packs, a partial resolution failure silently shrinks the scanned set — runLint never surfaces it, and report() will print "no violations found" for packs that were never actually scanned. For an enforcement lint whose whole purpose is a CI gate, silently skipping a pack defeats the tool's guarantee: a broken/unresolvable pack passes the check instead of failing loudly.
🛡️ Proposed fix — log the resolve failure like the manifest-load path does
for (cfg.plugins) |plugin| {
- const pack_dir = cache.resolvePlugin(arena, plugin, root) catch continue;
+ const pack_dir = cache.resolvePlugin(arena, plugin, root) catch |err| {
+ std.log.warn("labelle-assembler check: skipping pack '{s}': failed to resolve ({s})", .{ plugin.name, `@errorName`(err) });
+ continue;
+ };📝 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 discoverPacks(arena: std.mem.Allocator, io: std.Io, cfg: ProjectConfig, root: []const u8) ![]const Pack { | |
| var list: std.ArrayList(Pack) = .empty; | |
| for (cfg.plugins) |plugin| { | |
| const pack_dir = cache.resolvePlugin(arena, plugin, root) catch continue; | |
| var manifest = (plugin_manifest.loadPackFromDir(arena, pack_dir, plugin.name) catch |err| { | |
| // A malformed pack.labelle is a real problem, but the DAG gate | |
| // (`pack_validate`) is where that's reported at generate time; | |
| // the lint should not hard-fail on it. Skip and continue. | |
| std.log.warn("labelle-assembler check: skipping pack '{s}': {s}", .{ plugin.name, @errorName(err) }); | |
| continue; | |
| }) orelse continue; // not a pack (no pack.labelle) | |
| defer manifest.deinit(); | |
| fn discoverPacks(arena: std.mem.Allocator, io: std.Io, cfg: ProjectConfig, root: []const u8) ![]const Pack { | |
| var list: std.ArrayList(Pack) = .empty; | |
| for (cfg.plugins) |plugin| { | |
| const pack_dir = cache.resolvePlugin(arena, plugin, root) catch |err| { | |
| std.log.warn("labelle-assembler check: skipping pack '{s}': failed to resolve ({s})", .{ plugin.name, `@errorName`(err) }); | |
| continue; | |
| }; | |
| var manifest = (plugin_manifest.loadPackFromDir(arena, pack_dir, plugin.name) catch |err| { | |
| // A malformed pack.labelle is a real problem, but the DAG gate | |
| // (`pack_validate`) is where that's reported at generate time; | |
| // the lint should not hard-fail on it. Skip and continue. | |
| std.log.warn("labelle-assembler check: skipping pack '{s}': {s}", .{ plugin.name, `@errorName`(err) }); | |
| continue; | |
| }) orelse continue; // not a pack (no pack.labelle) | |
| defer manifest.deinit(); |
🤖 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/check_cmd.zig` around lines 113 - 124, The resolve failure in
discoverPacks is being swallowed by catch continue, which can hide skipped packs
from the lint scan. Update the cache.resolvePlugin path in discoverPacks to log
a warning with std.log.warn before continuing, matching the
manifest.loadPackFromDir error handling and including the plugin.name and error
name so runLint/report can accurately reflect partial scans.
Comment on lines
+356
to
+377
| for (ctx.global_facets) |gf| { | ||
| if (!std.mem.eql(u8, gf.name, name)) continue; | ||
| // The owner may construct its own facet freely. | ||
| if (gf.owner_prefix) |op| { | ||
| if (std.mem.eql(u8, op, ctx.current_prefix)) return; | ||
| } | ||
| // A pack that defines its OWN type of this name isn't touching the | ||
| // shared facet — skip to avoid a false positive on a name clash. | ||
| if (currentOwnsComponent(name, ctx)) return; | ||
| // Qualified (`foo.Locked{}`) is a field/namespaced access we can't | ||
| // confidently attribute to the facet — stay silent for precision. | ||
| if (ident_i > 0 and toks[ident_i - 1].tag == .period) return; | ||
|
|
||
| const msg = try std.fmt.allocPrint( | ||
| arena, | ||
| "pack '{s}' constructs the shared '.global' facet '{s}' directly (`{s}{{...}}`); global facets are multi-writer coordination primitives and must be mutated via their sanctioned API (e.g. `{s}.tryAcquire`), never a raw component write (RFC §6)", | ||
| .{ ctx.current_prefix, name, name, name }, | ||
| ); | ||
| try emit(arena, findings, .raw_global_facet_write, src, file, toks[ident_i].start, msg); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for consumer-side functions returning a candidate global facet type (e.g. Locked)
# to gauge how often the return-type shape occurs in real pack sources.
rg -nP '\)\s*Locked\s*\{' -g '*.zig'Repository: labelle-toolkit/labelle-assembler
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rule implementation and nearby helpers.
sed -n '320,390p' src/check.zig
# Find any parsing/token-context helpers that distinguish struct literals from type positions.
rg -n "currentOwnsComponent|global_facets|ident_i|tag == \\.period|\\.l_brace|\\.identifier|function|return type|type position" src/check.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 4204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where checkGlobalFacetWrite is called and what token pattern it sees.
sed -n '220,320p' src/check.zig
sed -n '390,470p' src/check.zig
# Show the existing tests around the global facet rule.
sed -n '580,730p' src/check.zig
# Look for any parsing helpers that would exclude function signatures/return types.
rg -n "fn .*checkGlobalFacetWrite|raw_global_facet_write|function declaration|return type|fn currentLock|\\.fn" src/check.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 12656
<name>{ should not flag function return types
scanSource treats any identifier followed by { as a facet write, so fn currentLock() Locked { ... } is reported even though Locked is only the return type. The current . guard only skips qualified names, so this still needs a bit more context disambiguation plus a regression test for fn ...() Locked {}.
🤖 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/check.zig` around lines 356 - 377, The raw global facet write check in
scanSource is overmatching function return types, since an identifier followed
by “{” is being treated as a facet construction even in signatures like fn
currentLock() Locked { ... }. Update the logic in the scanSource path around the
current facet-name matching and period guard to recognize function
declarations/return-type contexts and skip those cases before calling emit for
.raw_global_facet_write. Add a regression test covering fn ...() Locked {} so
the checker only flags actual facet writes, not return types.
Contributor
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.zig (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVersion-history comments are out of order.
The
v5block (lines 40-43,check) is listed before thev4block (lines 44-46,add), even thoughPROTOCOL_VERSIONis bumped to 5 here. Reading top-to-bottom now gives v2, v3, v5, v4 — confusing for anyone adding v6 later.✏️ Suggested reorder
-/// v5 (labelle-cli#270): added the `check` subcommand — the Packs -/// enforcement lint (RFC §6). Additive; the CLI's `labelle check` delegates -/// to it. The CLI still only *requires* protocol >= 3 (older binaries just -/// lack `check`), so this bump is informational. /// v4 (Packs `#271`): added the `add` subcommand (`add pack <name>` / /// `add feature <kind> <name>`). The CLI delegates pack/feature-unit /// scaffolding to the binary. +/// +/// v5 (labelle-cli#270): added the `check` subcommand — the Packs +/// enforcement lint (RFC §6). Additive; the CLI's `labelle check` delegates +/// to it. The CLI still only *requires* protocol >= 3 (older binaries just +/// lack `check`), so this bump is informational. pub const PROTOCOL_VERSION: u32 = 5;🤖 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/main.zig` around lines 40 - 47, The version-history comments above PROTOCOL_VERSION are out of chronological order, with the v5 note appearing before v4; reorder the adjacent comment blocks in src/main.zig so they read in ascending version order (older to newer) while keeping the PROTOCOL_VERSION constant and the existing descriptions intact, using the PROTOCOL_VERSION symbol as the anchor.
🤖 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/main.zig`:
- Around line 40-47: The version-history comments above PROTOCOL_VERSION are out
of chronological order, with the v5 note appearing before v4; reorder the
adjacent comment blocks in src/main.zig so they read in ascending version order
(older to newer) while keeping the PROTOCOL_VERSION constant and the existing
descriptions intact, using the PROTOCOL_VERSION symbol as the anchor.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds a new labelle-assembler check --project-root <path> subcommand that lints Packs §6 “enforcement net” violations by scanning pack source tokens and reporting rule-based findings with file/line/col, integrating it into the CLI dispatch and protocol versioning.
Changes:
- Introduces a
check.zigtoken-scanner implementing three enforcement rules (cross-pack registry access, raw.globalfacet writes, event direction inversions) with unit tests. - Adds
check_cmd.zigdriver to discover packs fromproject.labelle, build shared ownership/DAG context, run scans over pack directories, and report/exit non-zero on violations (with fixture e2e tests). - Wires the
checksubcommand intomain.zigusage/dispatch and updates protocol versioning.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/root.zig | Ensures the new check.zig module is included in the test import set. |
| src/main.zig | Adds check to usage + dispatch and bumps PROTOCOL_VERSION. |
| src/check.zig | Implements the pure token-scan lint rules and pack directory walker, with unit tests. |
| src/check_cmd.zig | Implements pack discovery/context building, reporting, and end-to-end fixture tests for check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+289
to
+292
|
|
||
| // Rule 3 — reference to a higher pack's qualified event. | ||
| try checkEventDirection(arena, findings, src, file, ctx, tk, name); | ||
| } |
Comment on lines
+40
to
+47
| /// v5 (labelle-cli#270): added the `check` subcommand — the Packs | ||
| /// enforcement lint (RFC §6). Additive; the CLI's `labelle check` delegates | ||
| /// to it. The CLI still only *requires* protocol >= 3 (older binaries just | ||
| /// lack `check`), so this bump is informational. | ||
| /// v4 (Packs #271): added the `add` subcommand (`add pack <name>` / | ||
| /// `add feature <kind> <name>`). The CLI delegates pack/feature-unit | ||
| /// scaffolding to the binary. | ||
| pub const PROTOCOL_VERSION: u32 = 4; | ||
| pub const PROTOCOL_VERSION: u32 = 5; |
Comment on lines
+452
to
+453
| while (try it.next(io)) |entry| { | ||
| const child = try std.fs.path.join(arena, &.{ dir_path, entry.name }); |
Comment on lines
+278
to
+286
| for (packs, 0..) |_, x| { | ||
| const visited = try arena.alloc(bool, n); | ||
| @memset(visited, false); | ||
| try dfsReach(arena, packs, &index_of, x, visited); | ||
| for (0..n) |r| { | ||
| if (r == x) continue; | ||
| if (visited[r]) try accum[r].append(arena, packs[x].prefix); | ||
| } | ||
| } |
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
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.
Adds
labelle-assembler check --project-root <path>— the AST/token half of the Packs enforcement "net" (labelle-cli#270). The CLI companion PR (labelle-cli#packs/check-lint) delegateslabelle checkto this subcommand.Part of the Packs initiative — umbrella labelle-toolkit/labelle-engine#651; RFC
Flying-Platform/flying-platform-labelledocs/RFC-packs.md§6 "Enforcement — isolation is structural, not documented". This is the interim lint (the net) layer; the compile guarantees (module isolation + thePackViewregistry partition) land engine-side (#652-remainder).Checks landed
cross-pack-registry-accessgetType("<other>__X")/entityHasNamed(…, "<other>__X")on a foreign namespace prefix;view(.{ForeignComponent})on an unambiguously foreign component<pack>__name (#440); the string form is exactly the escape hole module isolation leaves open (§6-1b)raw-global-facet-writeLocked{…}construction in a pack that doesn't own the facet, bypassingLocked.tryAcquireFacet{(construction) is distinct fromFacet.(the sanctioned API); owner + own-name-clash guards avoid FPsevent-direction-inversion<higher>__<event>, per thedepends_onDAGpack_validate.zigparses (#441); subscription detection is a qualified-name reference (it can't trace a subscription routed through an opaque value). The compile-time direction wall is future work, called out in the module doc-comment.Precision was the priority — every rule errs toward silence over a false positive (ambiguous component owner → skip; qualified
foo.Locked{}→ skip; non-pack prefixes likeengine__Position→ skip).Design
src/check.zig— pure,std-only token scanner over aContext(owner maps + DAG-derived "who is higher than me" set). 14 unit tests exercise each rule's positive + negative paths in-memory.src/check_cmd.zig— the driver: readsproject.labelle, discovers packs viaplugin_manifest.loadPackFromDir, resolves dirs viacache.resolvePlugin, builds the owner maps + reverse-transitivedepends_onclosure, walks each pack's.zigfiles, printsfile:line:col [rule] message, exits 1 on any finding (0 when clean / no packs). 2 fixture-based end-to-end tests (runLint) cover a violating three-pack tree and a clean one.main.zigdispatch + usage;PROTOCOL_VERSION→ 4 (informational — the CLI still only requires ≥3).Verification
zig build✓zig build test✓ — 46/46 steps, 1080/1084 passed (4 skipped, 0 failed); the new suites arecheck.zig(14) +check_cmd.zig(2 fixture).zig fmt --checkclean on all touched files.contracts/citizens/production,production depends_on citizens): the violating tree reports all three rules and exits 1; the cleaned tree reports "no violations" and exits 0.Deferred / honest limits
check.zigmodule comment..global-facet derivation defaults toLocked(the RFC's canonical example) plus acontractspack's own components; per-component.visibilityisn't parsed yet (it doesn't exist in the manifest — seemanifest.zig), so this is the pragmatic interim source.Summary by CodeRabbit
checkcommand to scan projects for Packs convention violations and report findings with precise file locations.labelle-assembler check --project-root <path>..globalfacet construction, and event-direction inversions.