Repository navigation
packs: labelle check — the §6 enforcement lint (Closes #270) - #273
Conversation
Add the `labelle check [dir]` subcommand: a thin CLI wrapper that delegates to `labelle-assembler check` (which owns the AST/token scan and the pack.labelle DAG machinery), mirroring how `generate`/`install` delegate. The assembler's exit code propagates, so `labelle check` drops straight into a CI gate (0 clean / 1 on violations). New file `src/cli/check.zig` (kept distinct to minimize overlap with the parallel `labelle add` work); wired into cli.zig's command enum + dispatch and the help text. Part of the Packs initiative (umbrella labelle-engine#651; RFC moca-tecnologia/flying-platform-labelle#561 §6). Requires labelle-assembler with the `check` subcommand (protocol v4).
|
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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changeslabelle check subcommand
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/cli/check.zig (1)
23-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for
cmdCheckarg parsing.Sibling command modules (
migrate.zig,audit.zig) exposeSpectest structs re-surfaced insrc/cli.zig's test aggregator;cmdCheck's dir-extraction, flag-forwarding, and too-many-args error path currently have no coverage.🤖 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/cli/check.zig` around lines 23 - 49, Add unit tests for cmdCheck’s argument parsing and error handling by covering the dir extraction, flag forwarding, and too-many-arguments path in src/cli/check.zig. Follow the pattern used by the Spec test structs in migrate.zig and audit.zig, and surface the new tests through src/cli.zig’s test aggregator so cmdCheck is exercised alongside the other command modules. Use cmdCheck as the entry point in the tests and verify its behavior for no dir, one dir with forwarded flags, and extra positional arguments.
🤖 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/cli/check.zig`:
- Around line 30-46: The cmdCheck argument parsing is forwarding reserved CLI
options and treats any non-dash token after a flag as the positional dir, so
check can inject a duplicate --project-root or misparse flag values. Update the
loop in cmdCheck to reserve --project-root for the CLI and handle option arity
explicitly, so only valid user flags go into forwarded while the dir is parsed
correctly. Keep the final argv assembly using the parsed dir and forwarded list,
but ensure --project-root is not re-forwarded when building argv.
---
Nitpick comments:
In `@src/cli/check.zig`:
- Around line 23-49: Add unit tests for cmdCheck’s argument parsing and error
handling by covering the dir extraction, flag forwarding, and too-many-arguments
path in src/cli/check.zig. Follow the pattern used by the Spec test structs in
migrate.zig and audit.zig, and surface the new tests through src/cli.zig’s test
aggregator so cmdCheck is exercised alongside the other command modules. Use
cmdCheck as the entry point in the tests and verify its behavior for no dir, one
dir with forwarded flags, and extra positional arguments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b6706ef-f36d-4d0a-9d90-50e497ccddbe
📒 Files selected for processing (3)
src/cli.zigsrc/cli/check.zigsrc/cli/help.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: fce9d77c71
ℹ️ 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".
| try argv.appendSlice(allocator, &.{ "--project-root", dir }); | ||
| try argv.appendSlice(allocator, forwarded.items); | ||
|
|
||
| try assembler_proc.runSubcommand(allocator, dir, "check", argv.items); |
There was a problem hiding this comment.
When labelle check runs with a project pinned to an older protocol-3 assembler (or the CLI-paired default before it is bumped), this generic runSubcommand path still accepts that binary and then invokes a subcommand it does not implement, so the new CI lint fails with an assembler “unknown subcommand” usage error instead of running. Since check is the first protocol-4 command, this path should either require protocol >=4 for this subcommand or ensure the resolved default/pin is upgraded before delegating.
Useful? React with 👍 / 👎.
Address two codex review findings on the `labelle add` PR. Finding 1 (protocol floor): reverting the global protocol minimum back to 3 and gating `add` at 4 per-subcommand. The prior global bump 3->4 rejected the auto-downloaded DEFAULT_ASSEMBLER_VERSION (0.40.0, protocol 3) before any command could run — a breaking change for every fresh install with no project pin. `minProtocolFor(subcommand)` is now a small lookup (floor = MIN_PROTOCOL = 3, `add` = 4) that `resolve`/`checkProtocol` consult; new gated subcommands add one line (e.g. `check` -> 5, #273) without raising the floor. Error message names the subcommand. Finding 2 (require a project): `add` is dispatched from the standalone switch, skipping the readProjectConfig guard project-scoped commands use, so `labelle add ...` in a non-project cwd would scaffold packs/components/ scripts there. `cmdAdd` now checks `config.projectExists(".")` and emits the shared "No project.labelle found" error (exit 1) before forwarding. Tests: minProtocolFor map (add=4, floor cmds=3, unknown=floor, gate>floor) and config.projectExists (present/absent/nonexistent). 301/301 pass.
* feat(#271): `labelle add pack` / `labelle add feature` subcommand Thin CLI forwarder for the Packs scaffold (RFC §7): parses `add ...` off argv and hands it verbatim to the assembler's `add` subcommand, same split as `labelle init`. Adds `add` to the Command enum + dispatch + arg parsing, a new `cli/add.zig`, and help text/examples. Bumps REQUIRED_PROTOCOL to 4 (the `add` subcommand needs the protocol-4 assembler). Closes #271. Part of #651. * fix(#272): per-subcommand protocol minimum + require project for `add` Address two codex review findings on the `labelle add` PR. Finding 1 (protocol floor): reverting the global protocol minimum back to 3 and gating `add` at 4 per-subcommand. The prior global bump 3->4 rejected the auto-downloaded DEFAULT_ASSEMBLER_VERSION (0.40.0, protocol 3) before any command could run — a breaking change for every fresh install with no project pin. `minProtocolFor(subcommand)` is now a small lookup (floor = MIN_PROTOCOL = 3, `add` = 4) that `resolve`/`checkProtocol` consult; new gated subcommands add one line (e.g. `check` -> 5, #273) without raising the floor. Error message names the subcommand. Finding 2 (require a project): `add` is dispatched from the standalone switch, skipping the readProjectConfig guard project-scoped commands use, so `labelle add ...` in a non-project cwd would scaffold packs/components/ scripts there. `cmdAdd` now checks `config.projectExists(".")` and emits the shared "No project.labelle found" error (exit 1) before forwarding. Tests: minProtocolFor map (add=4, floor cmds=3, unknown=floor, gate>floor) and config.projectExists (present/absent/nonexistent). 301/301 pass.
There was a problem hiding this comment.
Pull request overview
Adds a new labelle check [dir] CLI command that delegates to the companion labelle-assembler check subcommand, enabling packs §6 convention linting to be used as a CI gate via non-zero exit codes.
Changes:
- Adds
src/cli/check.zigimplementing thelabelle check [dir]delegation wrapper tolabelle-assembler check --project-root <dir>. - Wires the new
checkcommand intosrc/cli.zig(command enum, parsing, and dispatch). - Updates
src/cli/help.zighelp output to document the new subcommand.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/cli/help.zig | Adds check to CLI help text. |
| src/cli/check.zig | Implements argument handling + delegation to the assembler’s check subcommand. |
| src/cli.zig | Registers/parses/dispatches the new check command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Handle `labelle check [dir]`. `cmd_args` is everything after the | ||
| /// `check` token. The single optional positional is the project directory | ||
| /// (default `.`); any flags are forwarded to the assembler untouched so | ||
| /// future `check` flags need no CLI change. | ||
| pub fn cmdCheck(allocator: std.mem.Allocator, cmd_args: []const []const u8) !void { | ||
| var dir: []const u8 = "."; | ||
| var dir_set = false; | ||
|
|
||
| var forwarded: std.ArrayList([]const u8) = .empty; | ||
| defer forwarded.deinit(allocator); | ||
|
|
||
| for (cmd_args) |arg| { | ||
| if (std.mem.startsWith(u8, arg, "-")) { | ||
| try forwarded.append(allocator, arg); | ||
| } else if (!dir_set) { | ||
| dir = arg; | ||
| dir_set = true; | ||
| } else { | ||
| std.debug.print("labelle check: unexpected argument '{s}'\n", .{arg}); | ||
| std.debug.print(" usage: labelle check [dir]\n", .{}); | ||
| return error.TooManyArguments; | ||
| } | ||
| } |
…ng (#273) Sync `packs/check-lint` past main (#272 per-subcommand protocol floor, `add` subcommand) and finish the `labelle check` wiring: - cli.zig: resolve the Command-enum merge conflict keeping BOTH `add_cmd` and `check_cmd`; surface `check.ParseCheckArgsSpec` in the test aggregator. - assembler_proc.zig: un-stub the gate table entry `check` -> 5 so `runSubcommand(.., "check", ..)` rejects a protocol-<5 assembler up front (clear "needs protocol >= 5" error) instead of it failing with "unknown subcommand"; add a MinProtocolForSpec case for it. - check.zig: rewrite arg parsing into a pure, testable `parseCheckArgs`. Reserve `--project-root` (space + `=value` forms) so the CLI's own injection is never double-added and its value is not mistaken for the positional dir; accept at most one positional (TooManyArguments otherwise); forward all other flags untouched. Add ParseCheckArgsSpec unit tests (dir extraction, flag forwarding, project-root reservation, too-many-args error). zig build + zig build test green (312/312). Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Adds
labelle check [dir]— the Packs enforcement "net" (Phase 5): a static/AST scan over the game + its packs that reports the §6 convention rules the compiler can't (yet) enforce, and exits non-zero on violations so it drops straight into a CI gate.Closes #270. Part of the Packs initiative — umbrella labelle-toolkit/labelle-engine#651; RFC
Flying-Platform/flying-platform-labelledocs/RFC-packs.md§6.Architecture
The AST/token scan lives in the assembler (which already has the pack-scan +
pack.labelleDAG machinery from #439/#440/#441) — companion PR labelle-toolkit/labelle-assembler#486 adds thechecksubcommand. This CLI change is the thin driver:labelle check [dir]locates + delegates tolabelle-assembler check --project-root <dir>, mirroring howgenerate/installdelegate. The assembler's exit code propagates verbatim.src/cli/check.zigis a distinct file (per the ticket note, to minimize overlap with the parallellabelle add#271 work — only the command enum + one dispatch line touchcli.zig).Checks (implemented in the assembler PR)
getType("<other>__X")/entityHasNamed(…, "<other>__X")/view(.{ForeignComponent})on a foreign pack's name — high confidence..global-facet writes —Locked{…}bypassingLocked.tryAcquire— high confidence.depends_onDAG — good, best-effort (subscription detection is heuristic; the compile-time wall is future).Framework, fixtures, and honest deferral details are in the assembler PR.
Verification
zig build✓,zig build test✓ — 6/6 steps, 292/292 tests passed.zig fmt --checkclean.LABELLE_ASSEMBLERpointed at the companion build) against a synthetic three-pack game:labelle checkreports the violations and exits 1; the cleaned tree exits 0.Requires
An assembler with the
checksubcommand (protocol v4 — labelle-assembler#486). Against an older pinned assembler,labelle checksurfaces the assembler's "unknown subcommand" message;generate/build/runare unaffected (the CLI still only requires protocol ≥3).